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

Proposal: remove restriction that SpanContext must be a sealed/final class #970

Closed
anuraaga opened this issue Sep 18, 2020 · 8 comments · Fixed by #969
Closed

Proposal: remove restriction that SpanContext must be a sealed/final class #970

anuraaga opened this issue Sep 18, 2020 · 8 comments · Fixed by #969
Labels
area:api Cross language API specification issue priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory

Comments

@anuraaga
Copy link
Contributor

I think the spec that requires SpanContext is too limiting to language authors since it brings language concerns (class inheritance) into the cross-language spec. Languages may have a language-specific reason to allow a non-sealed SpanContext. For example, in Java, we have a need to hide the implementation of SpanContext from a user app to prevent compatibility issues between auto instrumentation and the app which without special care, can share classpath and have collisions. This can be done efficiently if SpanContext is an interface - two interfaces, one in the app, the other in the agent, but one implementation generated by the agent. Otherwise, we are stuck with two implementations, that copy between each other. In some sense, the fact that we rewrite the usage of SpanContext by a user through severe bytecode manipulation already seems to be incompliant with the spirit of this spec of wanting only one implementation of `SpanContext. This is an issue specific to Java instrumentation - I'd recommend the spec not be a blocker in creating solutions to such issues.

@anuraaga anuraaga added the spec:trace Related to the specification/trace directory label Sep 18, 2020
@Oberon00 Oberon00 added area:api Cross language API specification issue release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Sep 18, 2020
@Oberon00
Copy link
Member

Oberon00 commented Sep 22, 2020

I would vote for making this P1 @open-telemetry/technical-committee. We need a decision here. See also linked PR #969

@andrewhsu andrewhsu added the priority:p1 Highest priority level label Sep 25, 2020
@andrewhsu
Copy link
Member

from the issue triage mtg today with TC, marking this as P1 because we need to make a decision on this issue by end of day today @bogdandrutu

@yurishkuro
Copy link
Member

I've always been +1 to have SpanContext as interface. Do we have written explanation somewhere why that would be a bad thing?

@yurishkuro yurishkuro changed the title SpanContext must be a final class restriction limiting Proposal: remove restriction that SpanContext must be a sealed/final class Sep 25, 2020
@Oberon00
Copy link
Member

@yurishkuro See rejected OTEP open-telemetry/oteps#58

@yurishkuro
Copy link
Member

Thanks for the link. From what I can see there, there were no convincing arguments why SpanContext as interface would be bad, aside from the ref-counting in C++ argument, which seemed to be resolved in that discussion.

@carlosalberto
Copy link
Contributor

I'm fine with un-sealing it too.

@bogdandrutu
Copy link
Member

"Unsealing" is ok as long as the contract specifies:

  • TraceID - w3c definition
  • SpanID - w3c definition
  • etc.

One of the reason we wanted it to be "sealed" was that we don't want to deal with any random length for traceid/spanid/flags etc. And it was one of the most important thing we agreed on during the initial merging requirements between OC and OT.

@carlosalberto
Copy link
Contributor

One of the reason we wanted it to be "sealed" was that we don't want to deal with any random length for traceid/spanid/flags etc.

Sounds fair to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory
Projects
None yet
6 participants