Allow samplers to modify tracestate #988

lmolkova · 2020-09-22T17:38:52Z

Fixes #856

Changes

Added Tracestate to SamplingResult

iNikem · 2020-09-22T17:45:06Z

This seems to me to contradict this:

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.

By W3C Trace Context specification, Tracestate is immutable. So is SpanContext. How can samplers change immutable object inside another immutable object?

specification/trace/sdk.md

lmolkova · 2020-09-22T18:56:47Z

@iNikem

By W3C Trace Context specification, Tracestate is immutable. So is SpanContext. How can samplers change immutable object inside another immutable object?

Tracestate can be mutated.

This change enables following scenario

propagator parses trace-context (with tracestate)
sampler is invoked
- it can read a parent tracestate (possible today)
- make a copy and modify it
- add it to sampling result (not possible today)
open-telemetry sdk creates a new SpanContext with a tracestate from sampling result (not from parent context)

Let me know if this helps.

iNikem · 2020-09-22T19:00:40Z

Workflow does make sense, thank you. I still worry about implementation.

Does this mean that a new instance of SpanContext has to always be created after receiving sampling decision? Or SDK has to detect changes in TraceState?

Oberon00 · 2020-09-22T19:29:58Z

@iNikem Because of the new SpaId, a new SpanContext has to be created anyway. I expect implementation here to be trivial, replacing parent.tracestate with decision.tracestate

specification/trace/sdk.md

Oberon00

I think the new spec is quite confusing and unclear on what the sampler needs to do to use the parent tracestate without modification vs when the tracestate will be cleared. See my comment #988 (comment)

lmolkova · 2020-09-24T16:29:00Z

@Oberon00 I changed the wording as you suggested, thank you. Please have a look.

Oberon00 · 2020-09-24T16:32:18Z

Thank you for addressing my comments! I approved your PR, it looks great now!

arminru · 2020-09-25T13:25:32Z

CHANGELOG.md

@@ -29,6 +29,8 @@ New:
  * `exception.escaped` semantic span event attribute was added
    ([#784](https://github.com/open-telemetry/opentelemetry-specification/pull/784),
    [#946](https://github.com/open-telemetry/opentelemetry-specification/pull/946))
+- Allow samplers to modify tracestate
+  ([#856](https://github.com/open-telemetry/opentelemetry-specification/pull/988/))


Suggested change

([#856](https://github.com/open-telemetry/opentelemetry-specification/pull/988/))

([#988](https://github.com/open-telemetry/opentelemetry-specification/pull/988/))

arminru · 2020-09-25T13:27:57Z

specification/trace/sdk.md

+* A `Tracestate` that will be associated with the `Span` through the new
+  `SpanContext`.
+  Note: If the sampler returns an empty `Tracestate` here, the `Tracestate` will be cleared,
+  so samplers should normally return the passed-in `Tracestate` if they do not intend


Suggested change

so samplers should normally return the passed-in `Tracestate` if they do not intend

so samplers SHOULD return the `Tracestate` passed to them, if they do not intend

In a Note, we should not have upper-case requirements. If you want reword it, also drop the "Note:", and probably make it a MUST

arminru

@lmolkova Please add an entry in spec-compliance-matrix.md to keep track of implementations adopting the change.

arminru · 2020-09-25T13:30:25Z

CHANGELOG.md

@@ -29,6 +29,8 @@ New:
  * `exception.escaped` semantic span event attribute was added
    ([#784](https://github.com/open-telemetry/opentelemetry-specification/pull/784),
    [#946](https://github.com/open-telemetry/opentelemetry-specification/pull/946))
+- Allow samplers to modify tracestate


Suggested change

- Allow samplers to modify tracestate

- Add (modified) Tracestate to the return value of Samplers

Otherwise it sounds like something optional but this actually changed the interface and SIGs will have to implement this.

andrewhsu · 2020-09-25T15:17:27Z

from the issue triage meeting today with TC, this looks desirable, just needs changelog update which can be a followup @carlosalberto

tigrannajaryan · 2020-09-25T15:18:14Z

Merging as agreed during triage session since it has sufficient approvals.

- Fix CHANGELOG entry. - Use SHOULD for the tracestate change. - Add it to the compliance matrix.

Incorporating changes from open-telemetry/opentelemetry-specification#988

Add trace_state into SamplingResult. See open-telemetry/opentelemetry-specification#988 for details.

Allow samplers to modify tracestate, fix open-telemetry#856

43bb201

lmolkova requested a review from a team as a code owner September 22, 2020 17:38

lmolkova requested a review from a team September 22, 2020 17:38

lmolkova requested a review from a team as a code owner September 22, 2020 17:38

github-actions bot assigned jmacd Sep 22, 2020

lmolkova changed the title ~~Allow samplers to modify tracestate, fix #856~~ Allow samplers to modify tracestate Sep 22, 2020

changelog

d4b80a6

jkwatson reviewed Sep 22, 2020

View reviewed changes

specification/trace/sdk.md Outdated Show resolved Hide resolved

bogdandrutu approved these changes Sep 22, 2020

View reviewed changes

specification/trace/sdk.md Outdated Show resolved Hide resolved

review: tracestate is associated with Span through the context

d050af3

anuraaga mentioned this pull request Sep 23, 2020

Remove restriction that SpanContext must be a final class, specify creation. #969

Merged

iNikem approved these changes Sep 23, 2020

View reviewed changes

Oberon00 reviewed Sep 23, 2020

View reviewed changes

specification/trace/sdk.md Outdated Show resolved Hide resolved

carlosalberto self-requested a review September 24, 2020 00:19

carlosalberto approved these changes Sep 24, 2020

View reviewed changes

Oberon00 reviewed Sep 24, 2020

View reviewed changes

Update sdk.md

8e5bab6

Oberon00 approved these changes Sep 24, 2020

View reviewed changes

Merge branch 'master' into tracestateSampler

e73146e

Update sdk.md

652a1c2

Oberon00 added area:sampling Related to trace sampling area:sdk Related to the SDK spec:trace Related to the specification/trace directory labels Sep 25, 2020

arminru approved these changes Sep 25, 2020

View reviewed changes

arminru reviewed Sep 25, 2020

View reviewed changes

tigrannajaryan merged commit 5f4f92b into open-telemetry:master Sep 25, 2020

jtescher mentioned this pull request Sep 25, 2020

Allow samplers to modify tracestate open-telemetry/opentelemetry-rust#231

Closed

This was referenced Sep 25, 2020

Update SamplingResult to include the Tracestate open-telemetry/opentelemetry-java#1705

Closed

new SamplingResult spec makes it more expensive/difficult for Samplers that don't want to modify TraceState #1031

Open

carlosalberto added a commit to carlosalberto/opentelemetry-specification that referenced this pull request Sep 29, 2020

Follow up open-telemetry#988.

30f32f1

- Fix CHANGELOG entry. - Use SHOULD for the tracestate change. - Add it to the compliance matrix.

carlosalberto mentioned this pull request Sep 29, 2020

Follow up for #988. #1036

Merged

lmolkova pushed a commit to lmolkova/oteps that referenced this pull request Sep 29, 2020

Spec now allows samplers to modify tracestate

0721be6

Incorporating changes from open-telemetry/opentelemetry-specification#988

carlosalberto added a commit that referenced this pull request Sep 30, 2020

Follow up for #988. (#1036)

59fe1e7

TommyCpp added a commit to TommyCpp/opentelemetry-rust that referenced this pull request Oct 2, 2020

[trace] Allow ShouldSample implementation to modify trace state.

08298a2

Add trace_state into SamplingResult. See open-telemetry/opentelemetry-specification#988 for details.

TommyCpp mentioned this pull request Oct 2, 2020

[trace] Allow ShouldSample implementation to modify trace state. open-telemetry/opentelemetry-rust#237

Merged

jtescher pushed a commit to open-telemetry/opentelemetry-rust that referenced this pull request Oct 2, 2020

[trace] Allow ShouldSample implementation to modify trace state. (#237)

0841b9a

Add trace_state into SamplingResult. See open-telemetry/opentelemetry-specification#988 for details.

codeboten mentioned this pull request Oct 8, 2020

Allow samplers to modify tracestate open-telemetry/opentelemetry-python#1220

Closed

Oberon00 mentioned this pull request Oct 14, 2020

Move isRemote from SpanContext to Span #1086

Closed

cijothomas mentioned this pull request Jan 19, 2021

Samplers should be allowed to modify tracestate open-telemetry/opentelemetry-dotnet#1708

Closed

tarekgh mentioned this pull request Jan 11, 2022

Samplers should be allowed to modify tracestate dotnet/runtime#63652

Closed

spencerwilson mentioned this pull request May 16, 2022

Support TraceStates on SamplingResults open-telemetry/opentelemetry-js#2974

Closed

jship mentioned this pull request Sep 9, 2022

Tracestate arg should be mentioned in sampler's required args section #2783

Closed

Gmanboy added a commit to Gmanboy/opentelemetry-rust that referenced this pull request Aug 12, 2024

[trace] Allow ShouldSample implementation to modify trace state. (#237)

5db047f

Add trace_state into SamplingResult. See open-telemetry/opentelemetry-specification#988 for details.

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow samplers to modify tracestate #988

Allow samplers to modify tracestate #988

lmolkova commented Sep 22, 2020

iNikem commented Sep 22, 2020

lmolkova commented Sep 22, 2020 •

edited

Loading

iNikem commented Sep 22, 2020

Oberon00 commented Sep 22, 2020

Oberon00 left a comment

lmolkova commented Sep 24, 2020

Oberon00 commented Sep 24, 2020

arminru Sep 25, 2020

arminru Sep 25, 2020

Oberon00 Sep 25, 2020 •

edited

Loading

arminru left a comment

arminru Sep 25, 2020

andrewhsu commented Sep 25, 2020 •

edited

Loading

tigrannajaryan commented Sep 25, 2020

	([#856](https://github.com/open-telemetry/opentelemetry-specification/pull/988/))
	([#988](https://github.com/open-telemetry/opentelemetry-specification/pull/988/))

	so samplers should normally return the passed-in `Tracestate` if they do not intend
	so samplers SHOULD return the `Tracestate` passed to them, if they do not intend

	- Allow samplers to modify tracestate
	- Add (modified) Tracestate to the return value of Samplers

Allow samplers to modify tracestate #988

Allow samplers to modify tracestate #988

Conversation

lmolkova commented Sep 22, 2020

Changes

iNikem commented Sep 22, 2020

lmolkova commented Sep 22, 2020 • edited Loading

iNikem commented Sep 22, 2020

Oberon00 commented Sep 22, 2020

Oberon00 left a comment

Choose a reason for hiding this comment

lmolkova commented Sep 24, 2020

Oberon00 commented Sep 24, 2020

arminru Sep 25, 2020

Choose a reason for hiding this comment

arminru Sep 25, 2020

Choose a reason for hiding this comment

Oberon00 Sep 25, 2020 • edited Loading

Choose a reason for hiding this comment

arminru left a comment

Choose a reason for hiding this comment

arminru Sep 25, 2020

Choose a reason for hiding this comment

andrewhsu commented Sep 25, 2020 • edited Loading

tigrannajaryan commented Sep 25, 2020

lmolkova commented Sep 22, 2020 •

edited

Loading

Oberon00 Sep 25, 2020 •

edited

Loading

andrewhsu commented Sep 25, 2020 •

edited

Loading