-
Notifications
You must be signed in to change notification settings - Fork 225
KafkaChannel Tracing #1155
KafkaChannel Tracing #1155
Conversation
/assign @ian-mi |
42fca14
to
8600838
Compare
// If the span is not recorded, we just don't send these info | ||
span := trace.FromContext(ctx) | ||
defer span.End() | ||
if span.IsRecordingEvents() { |
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.
trace context propagation should probably happen regardless of whether events are recorded.
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 think what we do here is fine because it's similar to doing span.IsRecordingEvents()
: if you're not recording the span, there is no need to do the hard lifting for writing the span and reading it from the other side.
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.
Trace context is meant to be propagated regardless of sampling decision, otherwise there would be no need for a sampled flag. Here's zipkin's documentation on the reason for doing so: https://github.com/openzipkin/b3-propagation#why-send-trace-ids-with-a-reject-sampling-decision.
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.
/lgtm
During the CloudEvents WG meeting we just find out that this implementation is wrong, because it should not touch the |
I'm not sure what you mean by this, ce-traceparent is meant to be used for trace context propagation. |
But the event creator should set it, middlewares should not touch it. I'll send you the link to the PR to the spec with the clarification discussed today during the meeting. |
I propose for now to unblock this PR adding a @ian-mi How do you propose to serialize/deserialize |
9830660
to
2c7442d
Compare
Since now the spec is more specific, how do we want to proceed on this? |
2c7442d
to
20eaf42
Compare
0f5f483
to
6db0c4b
Compare
d32c8b0
to
8d7f6c3
Compare
/retest |
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
8d7f6c3
to
027fc28
Compare
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
The following is the coverage report on the affected files.
|
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: matzew, slinkydeveloper The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes #1139
Proposed Changes
Release Note