Skip to content

[trace] add Unmarshaler functionality to SpanContext and subfields #6738

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

jackgopack4
Copy link

@jackgopack4 jackgopack4 commented May 5, 2025

Adds Unmarshaler interface implementations for trace.SpanContext and subfields.

This closes #1819 and also will enable less custom code to close open-telemetry/opentelemetry-collector#11740 and open-telemetry/opentelemetry-collector#12212.

Unit tests added for new functionality.

@dmathieu
Copy link
Member

dmathieu commented May 6, 2025

This should close #1819

@pellared
Copy link
Member

pellared commented May 6, 2025

As far as I remember, we never had an agreement that we should add support for unmarshaling.

@jackgopack4
Copy link
Author

As far as I remember, we never had an agreement that we should add support for unmarshaling.

Thanks for this context; I work more in the collector than in the library so I was not aware of previous discussions regarding this matter. I just didn't understand why the data would support marshaling but not unmarshaling.

Additionally, it would help us with collector stability, specifically the observability requirements as the exporters create an internal span for tracking progress of requests through the pipeline via the trace.SpanContext object, which is lost if the traces are sent to the persistent queue via file storage extension or similar.

We should be able to work around this by implementing custom structs and helpers to imitate the unmarshal function, but it made sense to me to submit the PR for review in the library as well.

@pellared
Copy link
Member

pellared commented May 6, 2025

@jackgopack4, just to be clear. Ths was not a complain towards your work. Some of the conversation can be found here #1819. We appreciate your willingness to contribute.

I remember we did not want to add capabilities into the SDK that are not defined in the specification and could be a problem in future e.g. when unmarshaling from JSON feature is defined in the specification. The fact that we have MarshalJSON in these exported types we consider as a legacy.

@jackgopack4 jackgopack4 force-pushed the jackgopack4/add-unmarshaler-trace-spancontext branch from d3edc7d to e2435ee Compare May 6, 2025 14:21
@jackgopack4
Copy link
Author

thank you. If spec compliance is the concern, I've updated the JSON tags for the objects in this PR to match the OTel API Specification and added another note to the CHANGELOG to reflect it

@MrAlias MrAlias added this to the Subsequent v1.36.0 milestone May 15, 2025
@pellared pellared mentioned this pull request May 22, 2025
@jackgopack4
Copy link
Author

@MrAlias can you remind me what comments you want added to the trace.MarshalJSON function to land this change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Context through persistent queue SpanID, TraceID, KeyValue and other structs cannot be unmarshalled from JSON
4 participants