add support for kafka streams new processing api #1367

dermotmburke · 2023-03-21T16:45:23Z

…urke/brave into kafka-streams-new-processor-api

jeqo

Thanks @dermotmburke! This looks great!
I took an initial look and added some comments about naming, though I'm open to hear other opinions.

instrumentation/kafka-streams/src/main/java/brave/kafka/streams/KafkaStreamsTracing.java

instrumentation/kafka-streams/src/test/java/brave/kafka/streams/ITKafkaStreamsTracing.java

…s/KafkaStreamsTracing.java using the suggested naming convention Co-authored-by: Jorge Esteban Quilcate Otoya <jorge.quilcate@aiven.io>

…s/ITKafkaStreamsTracing.java using the suggested naming convention Co-authored-by: Jorge Esteban Quilcate Otoya <jorge.quilcate@aiven.io>

dermotmburke · 2023-03-30T13:06:02Z

Thanks @jeqo. Yeah the naming of the new TracingProcessor didn't seem that straight forward - but as you said the encapsulation prevents the name from leaking out so I suppose it doesn't matter that much. I merged your suggested changes.

jeqo

Thanks @dermotmburke. I guess you missed changing the file names :)

Once build is fixed and names aligned, we could wrap up this PR by updating the instrumentation/kafka-streams/README.md and RATIONALE.md. Let me know how you feel about adding this and if you need any help. Thanks again!

instrumentation/kafka-streams/src/main/java/brave/kafka/streams/NewTracingProcessor.java

instrumentation/kafka-streams/src/test/java/brave/kafka/streams/KafkaStreamsTest.java

dermotmburke · 2023-04-05T09:44:03Z

Apologies @jeqo I should have checked the PR changes a bit closer :)

I have updated the README.md and RATIONALE.md files as suggested. Wasn't entirely sure what changes to make so let me know if they make sense.

Thanks

jeqo

Thanks @dermotmburke! Left some minor comments to improve docs, but all looks good to me. Once fixed, it should be good to merge unless CI says otherwise :)

instrumentation/kafka-streams/RATIONALE.md

instrumentation/kafka-streams/README.md

Co-authored-by: Jorge Esteban Quilcate Otoya <jorge.quilcate@aiven.io>

dermotmburke · 2023-04-09T07:32:52Z

Sorry - been a bit too busy with family stuff over the Easter period to look at this. All looks good to me. Thanks

jeqo

thanks @dermotmburke !

Let's wait a bit if there's additional feedback from the the team, but LGTM!

dermotmburke · 2023-04-12T09:05:31Z

Hi @jeqo - looks like there was an error running the gRPC tests - is this a known issue?

jeqo · 2023-04-12T20:21:03Z

Thanks for noticing! I don't remember to have seeing it before. but it's definitely unrelated. Retrying now, let's see if persists

jcchavezs · 2023-04-13T17:05:41Z

instrumentation/kafka-streams/src/main/java/brave/kafka/streams/TracingV2Processor.java

+    } finally {
+      // Inject this span so that the next stage uses it as a parent
+      kafkaStreamsTracing.injector.inject(span.context(), record.headers());
+      if (error != null) span.error(error);


I wonder why this does not happen in the catch and if we should check for noop.

Interesting. Maybe we can discuss this in another issue (if it's an issue)?.
I can see we have follow this pattern on kafka-streams and kafka-clents:

brave/instrumentation/kafka-streams/src/main/java/brave/kafka/streams/TracingTransformer.java

Line 64 in 0e26fd7

if (error != null) span.error(error);

brave/instrumentation/kafka-clients/src/main/java/brave/kafka/clients/TracingCallback.java

Line 41 in f924f7f

if (exception != null) span.error(exception);

jcchavezs · 2023-04-13T17:06:00Z

instrumentation/kafka-streams/src/main/java/brave/kafka/streams/TracingV2FixedKeyProcessor.java

+    } finally {
+      // Inject this span so that the next stage uses it as a parent
+      kafkaStreamsTracing.injector.inject(span.context(), record.headers());
+      if (error != null) span.error(error);


same comment as below.

jcchavezs

Thanks @dermotmburke!

jeqo · 2023-04-14T08:35:52Z

Thanks @jcchavezs and @dermotmburke for the great work!

jeqo · 2023-04-14T08:37:01Z

Will ship a new release later today 🚀

dermotmburke · 2023-04-16T08:31:44Z

Thanks for all the support @jeqo

jcchavezs · 2023-04-16T08:47:50Z

<3

…

On Sun, 16 Apr 2023, 10:31 Dermot Burke, ***@***.***> wrote: Thanks for all the support @jeqo <https://github.com/jeqo> — Reply to this email directly, view it on GitHub <#1367 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXOYAWEJWEXNIQ62IGLQ3DXBOU7XANCNFSM6AAAAAAWCWU22A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

add support for kafka streams new processing api

478a05e

dermotmburke marked this pull request as draft March 21, 2023 16:45

Dermot Burke and others added 8 commits March 21, 2023 17:13

tidy up pull request

e4dbc0e

tidy up pull request

6463756

fix headers

001b7e7

fix integration tests by using salesforce kafka-junit lib

eb07b95

Merge branch 'master' into kafka-streams-new-processor-api

e974fa6

use latest version of charithe/kafka-junit

a1ec02c

Merge branch 'kafka-streams-new-processor-api' of github.com:dermotmb…

a1fea90

…urke/brave into kafka-streams-new-processor-api

tidy pr

31b971f

dermotmburke marked this pull request as ready for review March 23, 2023 17:40

dermotmburke mentioned this pull request Mar 23, 2023

KafkaStreamsTracing update to align with new Kafka Streams API #1365

Closed

Dermot Burke and others added 3 commits March 24, 2023 09:51

fix javadoc typo

ec9f569

fix typo in javadoc

63e1b31

Merge branch 'kafka-streams-new-processor-api' of github.com:dermotmb…

9c9b93e

…urke/brave into kafka-streams-new-processor-api

jeqo reviewed Mar 29, 2023

View reviewed changes

dermotmburke and others added 4 commits March 30, 2023 14:02

Update instrumentation/kafka-streams/src/main/java/brave/kafka/stream…

55a1aaf

…s/KafkaStreamsTracing.java using the suggested naming convention Co-authored-by: Jorge Esteban Quilcate Otoya <jorge.quilcate@aiven.io>

Update instrumentation/kafka-streams/src/main/java/brave/kafka/stream…

91dd168

…s/KafkaStreamsTracing.java using the suggested naming convention Co-authored-by: Jorge Esteban Quilcate Otoya <jorge.quilcate@aiven.io>

Update instrumentation/kafka-streams/src/test/java/brave/kafka/stream…

b2d575b

…s/ITKafkaStreamsTracing.java using the suggested naming convention Co-authored-by: Jorge Esteban Quilcate Otoya <jorge.quilcate@aiven.io>

Update instrumentation/kafka-streams/src/test/java/brave/kafka/stream…

9bb2b3b

…s/ITKafkaStreamsTracing.java using the suggested naming convention Co-authored-by: Jorge Esteban Quilcate Otoya <jorge.quilcate@aiven.io>

jeqo requested changes Apr 4, 2023

View reviewed changes

instrumentation/kafka-streams/src/main/java/brave/kafka/streams/NewTracingProcessor.java Outdated Show resolved Hide resolved

instrumentation/kafka-streams/src/test/java/brave/kafka/streams/KafkaStreamsTest.java Outdated Show resolved Hide resolved

implement PR suggestions to fix naming issues and docs

bad9919

dermotmburke requested a review from jeqo April 5, 2023 17:23

jeqo approved these changes Apr 5, 2023

View reviewed changes

instrumentation/kafka-streams/RATIONALE.md Outdated Show resolved Hide resolved

instrumentation/kafka-streams/RATIONALE.md Show resolved Hide resolved

instrumentation/kafka-streams/README.md Show resolved Hide resolved

jcchavezs requested a review from shakuzen April 5, 2023 21:40

dermotmburke and others added 2 commits April 9, 2023 08:31

Update instrumentation/kafka-streams/RATIONALE.md

d7faede

Co-authored-by: Jorge Esteban Quilcate Otoya <jorge.quilcate@aiven.io>

Update instrumentation/kafka-streams/README.md

2154e00

Co-authored-by: Jorge Esteban Quilcate Otoya <jorge.quilcate@aiven.io>

jeqo approved these changes Apr 9, 2023

View reviewed changes

jcchavezs reviewed Apr 13, 2023

View reviewed changes

jcchavezs approved these changes Apr 14, 2023

View reviewed changes

jeqo merged commit c455de9 into openzipkin:master Apr 14, 2023

dermotmburke deleted the kafka-streams-new-processor-api branch April 16, 2023 08:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for kafka streams new processing api #1367

add support for kafka streams new processing api #1367

dermotmburke commented Mar 21, 2023 •

edited by jeqo

Loading

jeqo left a comment

dermotmburke commented Mar 30, 2023

jeqo left a comment

dermotmburke commented Apr 5, 2023

jeqo left a comment

dermotmburke commented Apr 9, 2023

jeqo left a comment

dermotmburke commented Apr 12, 2023

jeqo commented Apr 12, 2023

jcchavezs Apr 13, 2023

jeqo Apr 13, 2023

jcchavezs Apr 13, 2023

jcchavezs left a comment

jeqo commented Apr 14, 2023

jeqo commented Apr 14, 2023

dermotmburke commented Apr 16, 2023

jcchavezs commented Apr 16, 2023 via email

add support for kafka streams new processing api #1367

add support for kafka streams new processing api #1367

Conversation

dermotmburke commented Mar 21, 2023 • edited by jeqo Loading

jeqo left a comment

Choose a reason for hiding this comment

dermotmburke commented Mar 30, 2023

jeqo left a comment

Choose a reason for hiding this comment

dermotmburke commented Apr 5, 2023

jeqo left a comment

Choose a reason for hiding this comment

dermotmburke commented Apr 9, 2023

jeqo left a comment

Choose a reason for hiding this comment

dermotmburke commented Apr 12, 2023

jeqo commented Apr 12, 2023

jcchavezs Apr 13, 2023

Choose a reason for hiding this comment

jeqo Apr 13, 2023

Choose a reason for hiding this comment

jcchavezs Apr 13, 2023

Choose a reason for hiding this comment

jcchavezs left a comment

Choose a reason for hiding this comment

jeqo commented Apr 14, 2023

jeqo commented Apr 14, 2023

dermotmburke commented Apr 16, 2023

jcchavezs commented Apr 16, 2023 via email

dermotmburke commented Mar 21, 2023 •

edited by jeqo

Loading