Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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/segmentio/kafka-go: add implementation add tracing for kafka writer and kafka reader #1152
contrib/segmentio/kafka-go: add implementation add tracing for kafka writer and kafka reader #1152
Changes from 17 commits
7ffd45f
f3c0482
d956bf2
60ae8d5
88ed7bc
418180d
4a7ac09
ddfae80
bb04e82
044b55c
84fbf18
388658f
f1454cd
96ff7ba
adcbe33
37eba0e
2cbfdee
ceaf249
947d5df
8de6e46
2d9aa54
3edfca6
bb0ab40
e6c72d3
967d212
75551e4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this type need to be exported? I don't think users need to interact directly with this and an unexported type can still conform to an interface.
Let's keep this unexported if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is not useful. We should point out that this implements the
ddtrace.TextMapWriter
interface.Same for
ForEachKey
(only for theTextMapReader
interface)We do this with
TextMapCarrier
. See:dd-trace-go/ddtrace/tracer/textmap.go
Lines 53 to 66 in 5b6e61b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, we should add just a bit more info for the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? What does wrapping kafka.Writer accomplish? We should add a little more to this documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about something like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not able to get these instructions to work, and anyway we don't want users to have to edit the tests in order to run them locally.
Let's either fix these instructions to bring up a kafka such that tests pass without modification (should be possible since 9092 is not a protected port) or remove these instructions.