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

contrib/confluentinc/confluent-kafka-go/kafka: Use kafka headers as c… #943

Merged
merged 1 commit into from
Jul 14, 2021

Conversation

hexa00
Copy link
Contributor

@hexa00 hexa00 commented Jun 2, 2021

…ontext when producing

Before this patch when producing a message, the general producer context was
used when producing messages.

This made it impossible to attach a parent span to a produced message.

With this patch messages headers are checked when producing messages for a span
context and the span is created as a child if a parent span is present.

Also StartSpanFromContext is changed to just StarSpan since using the global
consumer/producer context has no use.

Fixed #941

@hexa00 hexa00 force-pushed the fix-kafka-context-passing branch from 742c291 to 2085592 Compare June 4, 2021 14:18
@knusbaum knusbaum added this to the 1.32.0 milestone Jun 29, 2021
Copy link
Contributor

@knusbaum knusbaum left a comment

Choose a reason for hiding this comment

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

Hi, @hexa00
We should include clear examples of how to use this, as well as tests for the behavior.

We also need to maintain the existing behavior for anyone using the WithContext option. IMO, we should mark WithContext deprecated.

@hexa00
Copy link
Contributor Author

hexa00 commented Jun 29, 2021

@knusbaum thank you, a few questions:

  • Where should the examples be ? Would the tests work as examples ?
  • OK to maintain the behavior and mark as deprecated , is a comment with Deprecated: the proper way ?

@knusbaum
Copy link
Contributor

@hexa00

Sorry, I should have been clearer. :)

Thanks.

@hexa00
Copy link
Contributor Author

hexa00 commented Jun 29, 2021

@knusbaum great thanks for the info, I'll work on that today

@hexa00 hexa00 force-pushed the fix-kafka-context-passing branch 2 times, most recently from 2316c21 to 80bbf8d Compare June 29, 2021 19:02
@hexa00
Copy link
Contributor Author

hexa00 commented Jun 29, 2021

@knusbaum this PR is ready for a second look:

Changes:

  • Example / test for the new behavior
  • Test for the old behavior
  • Deprecated WithContext()
  • Kept the old behavior if a context containing a span is set on the producer
  • Rebased on latest v1

Let me know what you think, thanks! :)

Copy link
Contributor

@knusbaum knusbaum left a comment

Choose a reason for hiding this comment

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

@hexa00 Looks great. Thanks for making those changes.

Just one more change to the new example file and we should be good.

@@ -0,0 +1,80 @@
package kafka_test
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the license header to this file.

…ontext when producing

Before this patch when producing a message, the general producer context was
used when producing messages.

This made it impossible to attach a parent span to a produced message.

With this patch messages headers are checked when producing messages for a span
context and the span is created as a child if a parent span is present.

This patch deprecates the WithContext() option for consumer and producers.

Fixed DataDog#941
@hexa00 hexa00 force-pushed the fix-kafka-context-passing branch from a6fd2a1 to de6aa0a Compare July 8, 2021 14:52
@hexa00
Copy link
Contributor Author

hexa00 commented Jul 8, 2021

@knusbaum indeed , fixed and rebased

Copy link
Contributor

@knusbaum knusbaum left a comment

Choose a reason for hiding this comment

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

Looks good. 👍

@knusbaum knusbaum merged commit 5c1ff94 into DataDog:v1 Jul 14, 2021
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.

contrib/confluentinc/confluent-kafka-go Can't pass context per message when producing
2 participants