-
Notifications
You must be signed in to change notification settings - Fork 534
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
feat: OpenTracing propagator #318
Conversation
Codecov Report
@@ Coverage Diff @@
## main #318 +/- ##
==========================================
+ Coverage 95.44% 95.48% +0.04%
==========================================
Files 115 119 +4
Lines 6126 6273 +147
Branches 597 609 +12
==========================================
+ Hits 5847 5990 +143
- Misses 279 283 +4
|
propagators/opentelemetry-propagator-opentracing/test/OpenTracingPropagator.test.ts
Outdated
Show resolved
Hide resolved
propagators/opentelemetry-propagator-opentracing/src/OpenTracingPropagator.ts
Outdated
Show resolved
Hide resolved
propagators/opentelemetry-propagator-opentracing/src/OpenTracingPropagator.ts
Outdated
Show resolved
Hide resolved
propagators/opentelemetry-propagator-opentracing/src/OpenTracingPropagator.ts
Show resolved
Hide resolved
export const OT_SAMPLED_HEADER = 'ot-tracer-sampled'; | ||
export const OT_BAGGAGE_PREFIX = 'ot-baggage-'; | ||
|
||
const FIELDS = [OT_TRACE_ID_HEADER, OT_SPAN_ID_HEADER, OT_SAMPLED_HEADER]; |
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.
The multi-header baggage breaks the fields usecase. Clearing fields out of an object won't work if we don't know all the fields that were used. Saving used fields might result in a memory leak. Not sure how to resolve this. It should at least be documented.
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 added a comment further down above the fields()
method.
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 this should be also mentioned in readme similar as the restrictions regarding traceId which are already there.
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.
can we do opposite then, keep the names of fields that should be there and clear all the rest that are not on that list?
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 don't think that we actually use the fields
method in the API or SDK, but it is there for users or instrumentation authors to use. I documented the special case on the readme. The specification seems to hint at this possibility as well:
Observe that some Propagators may define, besides the returned values, additional fields with variable names. To get a full list of fields for a specific carrier object, use the Keys operation.
propagators/opentelemetry-propagator-opentracing/src/OpenTracingPropagator.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Gerhard Stöbich <[email protected]>
…ngPropagator.ts Co-authored-by: Daniel Dyla <[email protected]>
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
Which problem is this PR solving?
ot-tracer
) header format.Short description of the changes
OpenTracingPropagator
that handles context propagation for theot-tracer*
headers.