-
Notifications
You must be signed in to change notification settings - Fork 417
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
Comments
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 |
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. |
@jsuereth We would like to part these changes for next major release of otel-cpp after 1.0.0:
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. |
There's some open discussions going on around 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. |
Yes, that is doable with small restructuring of code, I think we are just slightly non-compliant this way :) |
The specification api states:
Here's the current API: https://github.com/open-telemetry/opentelemetry-cpp/blob/main/api/include/opentelemetry/trace/span.h#L67
The text was updated successfully, but these errors were encountered: