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

TraceState is mutated instead returning a new TraceState #1596

Closed
Flarna opened this issue Oct 14, 2020 · 9 comments · Fixed by #1597
Closed

TraceState is mutated instead returning a new TraceState #1596

Flarna opened this issue Oct 14, 2020 · 9 comments · Fixed by #1597
Assignees
Labels
bug Something isn't working
Projects

Comments

@Flarna
Copy link
Member

Flarna commented Oct 14, 2020

According to spec TraceState should be immutable and all mutating operations MUST return a new TraceState.

The actual implementation of e.g. set mutates the TraceState and returns void.

This has impact to the API which defines the interface for set to return void.

I think TraceState should be implemented similar then Context to create a new instance and copy key/values first.

@Flarna Flarna added the bug Something isn't working label Oct 14, 2020
@dyladan dyladan self-assigned this Oct 14, 2020
@Flarna
Copy link
Member Author

Flarna commented Oct 14, 2020

The use of TraceState together with HttpTraceContext seems to be problematic.

HttpTraceContext simply serializes spanContext.traceState.

Therefore the caller of inject is responsible to get the needed data into spancContext.traceState before calling inject().

But the spec tells that SpanContext is immutable therefore it's not allowed to set a the new TraceState.

What is the correct way to get an updated TraceState into a span?

@dyladan
Copy link
Member

dyladan commented Oct 14, 2020

The use of TraceState together with HttpTraceContext seems to be problematic.

HttpTraceContext simply serializes spanContext.traceState.

Therefore the caller of inject is responsible to get the needed data into spancContext.traceState before calling inject().

But the spec tells that SpanContext is immutable therefore it's not allowed to set a the new TraceState.

What is the correct way to get an updated TraceState into a span?

I think this is an issue with the spec, not our specific implementation of it. Fortunately for us, nothing in JS is truly immutable 😉.

@Flarna
Copy link
Member Author

Flarna commented Oct 14, 2020

Spec tells Please note, since SpanContext is immutable, it is not possible to update SpanContext with a new TraceState. Such changes then make sense only right before SpanContext propagation or telemetry data exporting. In both cases, Propagators and SpanExporters may create a modified TraceState copy before serializing it to the wire..

From spec point of view it's allowed for a Propagator to create a new TraceState but it renders HttpTraceContext useless for all users having the need of a modified TraceState.

@bogdandrutu
Copy link
Member

@dyladan TraceState should also be immutable (do not offer mutators like set and unset, I know user can still access the internal member and change that) in order to have an immutable SpanContext (a.k.a SpanReference).

The mutators must either return a new object (copying everything from the parent) or you can use a Builder pattern or something similar that is idiomatic in JS

@dyladan dyladan added this to To do issues in GA Burndown via automation Oct 14, 2020
@Flarna
Copy link
Member Author

Flarna commented Oct 14, 2020

So in short to get my changes injected I have to:

  • create a new TraceState
  • create a new SpanContext which references above TraceState
  • create a new Context which includes above SpanContext as current Span and pass it to inject

I think the last step is not possible as a SpanContext can't be converted into a Span. Besides that API offers nothing to put a Span on Context - this is done within the SDK/Tracer.

@dyladan
Copy link
Member

dyladan commented Oct 14, 2020

So in short to get my changes injected I have to:

  • create a new TraceState
  • create a new SpanContext which references above TraceState
  • create a new Context which includes above SpanContext as current Span and pass it to inject

I think the last step is not possible as a SpanContext can't be converted into a Span. Besides that API offers nothing to put a Span on Context - this is done within the SDK/Tracer.

After #1589 I believe step 3 is actually possible now.

@Flarna
Copy link
Member Author

Flarna commented Oct 14, 2020

Ah yes, I looked into 0.11-
Quite a lot to do for just getting one more element propagated via TraceState.

@bogdandrutu
Copy link
Member

Usually the TraceState should be changed when the Span is created not just before propagation, probably this is a bit of a unexpected behavior.

@Flarna
Copy link
Member Author

Flarna commented Oct 14, 2020

@bogdandrutu As far as I understand spans are created by calling tracer.startSpan() and there is no parameter to define a new value for the TraceState used for the new span - it uses that one from parent or none.

Therefore I have to either "fake" a parent span with the new TraceState or create a dummy span for propagator as described above.

Another options would be to use a custom Tracer or a custom Propagtor instead that ones from Otel to avoid some of the intermediate objects.

Or did I miss something?

GA Burndown automation moved this from To do issues to Done Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
GA Burndown
  
Done
Development

Successfully merging a pull request may close this issue.

3 participants