Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add KafkaProducer post-processor to enable extensions from other libraries #90

Merged
merged 6 commits into from
Oct 14, 2024

Conversation

sszp
Copy link
Contributor

@sszp sszp commented Sep 6, 2024

Context

I'd like to enable async traces for messages sent out with tw-tkms.

To able to use OpenTelemetry's KafkaTelemetry class from the tw-observability-base, I'm adding a new KafkaProducer post-processor interface that can be used from tw-observability-base to call the KafkaTelemetry:wrap() to create the necessary async traces for all message sent out with tw-tkms.

Checklist

@sszp sszp marked this pull request as ready for review September 20, 2024 14:52
@sszp sszp requested a review from a team as a code owner September 20, 2024 14:52
@tw-peeterkarolin
Copy link
Contributor

I think it would make sense to play this through with a snapshot version in combination with tw-observability-base to understand how the whole flow will work.
Just using KafkaTelemetry:wrap() won't be enough, because that that will work with normal use cases where the OpenTelemetry context is configured and available for the KafkaTelemetry wrapper to use. In tw-tkms polling the messages from the DB and publishing them to Kafka will happen in a separate poller thread, so it will need to initialise the OpenTelemetry context as well, before KafkaTelemetry can work.

@sszp
Copy link
Contributor Author

sszp commented Sep 25, 2024

I think it would make sense to play this through with a snapshot version in combination with tw-observability-base to understand how the whole flow will work. Just using KafkaTelemetry:wrap() won't be enough, because that that will work with normal use cases where the OpenTelemetry context is configured and available for the KafkaTelemetry wrapper to use. In tw-tkms polling the messages from the DB and publishing them to Kafka will happen in a separate poller thread, so it will need to initialise the OpenTelemetry context as well, before KafkaTelemetry can work.

I plan to look into that. At first I plan to see if I can use ITkmsMessageDecorator to add headers to the message and get those headers from the message using ITkmsMessageInterceptors just before it's sent out using the Kafka Client to initialise the OpenTelemetry context before sending.

normanma-tw
normanma-tw previously approved these changes Oct 8, 2024
Copy link
Contributor

@normanma-tw normanma-tw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once the snapshot version is cleaned up + optionally update the date of the changelog

@@ -51,4 +49,9 @@ public List<ITkmsMessageDecorator> messageDecorators() {
return Collections.emptyList();
}

@Bean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably would be better to declare the autowire usage as
@Autowired(required = false)
Then you don't need to explicitly provide an empty list bean.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed that.

@sszp
Copy link
Contributor Author

sszp commented Oct 11, 2024

LGTM once the snapshot version is cleaned up + optionally update the date of the changelog
That's done now. @normanma-tw

@sszp sszp merged commit 0c6c711 into master Oct 14, 2024
15 checks passed
@sszp sszp deleted the RDP-3392 branch October 14, 2024 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants