This repository has been archived by the owner on Oct 23, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
owais
commented
Apr 14, 2021
•
edited
Loading
edited
owais
force-pushed
the
sarama-improve-async-producer
branch
from
April 14, 2021 08:48
44af612
to
451ba5b
Compare
Depends on #119 |
owais
force-pushed
the
sarama-improve-async-producer
branch
from
April 14, 2021 08:53
451ba5b
to
efb1c31
Compare
owais
force-pushed
the
sarama-improve-async-producer
branch
2 times, most recently
from
April 14, 2021 08:55
057737e
to
e90170b
Compare
pellared
reviewed
Apr 14, 2021
owais
force-pushed
the
sarama-improve-async-producer
branch
4 times, most recently
from
April 14, 2021 12:10
d04ae01
to
5ae1b87
Compare
pellared
reviewed
Apr 14, 2021
pellared
reviewed
Apr 14, 2021
nit: please update the PR description 😉 |
owais
force-pushed
the
sarama-improve-async-producer
branch
2 times, most recently
from
April 14, 2021 13:49
8474efb
to
3745191
Compare
Sarama instrumentation was identifying spans with a non-unique and unelibale key which caused a number of issues with span reporting as some spans were not being finished. This commit replaces the said key with the span ID. This span ID is extracted from the kafka message headers and used to identify any pending spans. This requires Kafka (not sarama) version 0.11.0 or newer which was released in 2017.
owais
force-pushed
the
sarama-improve-async-producer
branch
from
April 14, 2021 13:53
3745191
to
649d12e
Compare
pellared
reviewed
Apr 14, 2021
c3 := &externalSpanContext{} | ||
assert.Equal(t, SpanID(c3), uint64(0)) | ||
assert.Equal(t, TraceID(c3), uint64(0)) | ||
|
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.
you can remove this empty line 😉
Co-authored-by: Robert Pająk <[email protected]>
pellared
reviewed
Apr 14, 2021
c3 := &externalSpanContext{} | ||
assert.Equal(t, SpanID(c3), uint64(0)) | ||
assert.Equal(t, TraceID(c3), uint64(0)) | ||
|
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.
probably you could also add:
var c4 SpanContext
assert.Equal(t, SpanID(c4), uint64(0))
assert.Equal(t, TraceID(c4), uint64(0))
also I like table-driven tests for tests like this https://blog.golang.org/subtests
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 gonna write a loop for 3 items 😆
pellared
approved these changes
Apr 14, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.