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

Span Parent should be determined from Context (and take this as a parameter. #955

Closed
jsuereth opened this issue Aug 19, 2021 · 5 comments · Fixed by #969
Closed

Span Parent should be determined from Context (and take this as a parameter. #955

jsuereth opened this issue Aug 19, 2021 · 5 comments · Fixed by #969
Labels
spec-compliance Not compliant to OpenTelemetry specs

Comments

@jsuereth
Copy link
Contributor

The specification api states:

The parent Context or an indication that the new Span should be a root Span. The API MAY also have an option for implicitly using the current Context as parent as a default behavior. This API MUST NOT accept a Span or SpanContext as parent, only a full Context.

The semantic parent of the Span MUST be determined according to the rules described in Determining the Parent Span from a Context.

Here's the current API: https://github.com/open-telemetry/opentelemetry-cpp/blob/main/api/include/opentelemetry/trace/span.h#L67

@jsuereth jsuereth added the spec-compliance Not compliant to OpenTelemetry specs label Aug 19, 2021
@lalitb
Copy link
Member

lalitb commented Aug 27, 2021

Good point. This was also observed as part of initial review for compliance matrix ( #717 ), but then few other languages ( at-least opentelemetry-dotnet ) also accepts SpanContext as parent, so we didn't do any change.
This would be a breaking change for existing customers, so need to plan it better.

@lalitb
Copy link
Member

lalitb commented Aug 28, 2021

As this is API breaking change, and other language(s) also have similar implementation, would like to discuss this in next community meeting before any changes.

@lalitb
Copy link
Member

lalitb commented Sep 2, 2021

@jsuereth We would like to part these changes for next major release of otel-cpp after 1.0.0:

  • It's a major API change, and we would like to avoid any such change while in Release Candidate.
  • There would be quite a few internal and external customers using the current RC in production, with expectation of no critical breaking change being added in 1.0.0.
  • There hasn't been any complaint from the existing customers regarding this deviation from otel-specs. Which means current api doesn't make things hard for them.

Unless we are missing any feature by passing parent's SpanContext, or making it difficult for applications to use the API, we would like to stick with current implementation, and park this change for next major otel-cpp release.

Would like to know if you see any issue here.

@jsuereth
Copy link
Contributor Author

jsuereth commented Sep 2, 2021

There's some open discussions going on around Baggage and Span.attributes that not having this feature will prevent us fixing going forward.

I understand not wanting to break customers. Are you able to provide this fix in a non-breaking fashion? I believe you should be able to. I think it's ok to have a legacy APi to avoid breakage, but given 1.0.0 is NOT out nor is their assumed compatibility prior to 1.0, you should be able to fix this now. The reason we do this review now is to finalize the 1.0 API prior to stabilizing.

@lalitb
Copy link
Member

lalitb commented Sep 3, 2021

Are you able to provide this fix in a non-breaking fashion? I believe you should be able to.

Yes, that is doable with small restructuring of code, I think we are just slightly non-compliant this way :)
Have raised PR for this : #969

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec-compliance Not compliant to OpenTelemetry specs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants