-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Feature: AddLinks() to spans. #3919
Comments
Should the |
Blocked on the specification stabilizing this feature. |
For clarity, it seems to be blocked on |
Thanks for the xref, much appreciated! Is there any room to discuss supporting experimental spec features in this SDK? Maybe under an isolated module that could clearly/cleanly document that functions in such a module are subject to breaking changes? I can understand a hesitancy to implementing experimental spec features generally, and especially for features that are otherwise accomplishable through non-experimental means, but
|
One problem is to make it "experimental" in SDK, the other is to make it available for API users.
Can you propose a design? |
I may well be underthinking the implications, but if
Might end up out of my depth here, but I can try to put together something concrete in the next couple of days. I'll expand on this when I have more time later today, but in the very broadest strokes, this is what I was thinking: module go.opentelemetry.io/otel/trace/unstable
func AddLinks(...) {
...
} |
This commit introduces a new ExperimentalSpan interface which aims to allow the API and SDK to support experimental features in a way that clearly communicates to users that they are opting into an API surface that is still considered experimental by the spec and thus may break, change, or be outright removed with minimal notice. In the case of `AddLinks()`, users can explicitly type-check for the new interface, making it obvious that they are interacting with an API surface that is considered experimental: ```go if s, ok := span.(trace.ExperimentalSpan); ok { s.AddLinks(links...) } ``` See: open-telemetry#3919 Signed-off-by: Austin Drenski <austin@austindrenski.io>
This commit introduces a new ExperimentalSpan interface which aims to allow the API and SDK to support experimental features in a way that clearly communicates to users that they are opting into an API surface that is still considered experimental by the spec and thus may break, change, or be outright removed with minimal notice. In the case of `AddLinks()`, users can explicitly type-check for the new interface, making it obvious that they are interacting with an API surface that is considered experimental: ```go if s, ok := span.(trace.ExperimentalSpan); ok { s.AddLinks(links...) } ``` See: open-telemetry#3919 Signed-off-by: Austin Drenski <austin@austindrenski.io>
@pellared As mentioned before, and at risk of ending up out of my depth, I took a first pass at how this idea might work in practice and opened #4889. In my earlier comments I mentioned a separate module, but after getting into the code, it seemed more intuitive and explicit to hide the experimental surfaces behind a new interface that will force users to explicitly type-check before calling into: if s, ok := span.(trace.ExperimentalSpan); ok {
s.AddLinks(links...)
} |
@austindrenski how will you handle the specification changing the method signature or having it removed entirely in your proposal? You will be adding a type and method to stable modules and those objects will need to persist indefinitely according to our versioning and compatibility guarantees. |
How about s, ok := span.(interface {
AddLinks(links ...Link)
})
if ok {
s.AddLinks(links...)
} This feature would need to be documented in the SDK docs and clearly defined as experimental. |
On the other side, I do not see a lot of demand for this feature. Therefore, I think it may be better to wait until it is stable in the OTel Specification. |
@MrAlias wrote #3919 (comment):
Will readily admit that I don't have an answer for this^. I had hoped that isolating the exposure to a separate interface would be sufficient, but it sounds like this project has stricter versioning guarantees than I previously understood. If I'm understanding the versioning issue correctly, then maybe I need to keep poking at the idea of an explicitly non-stable module where we could implement these features without the same compatibility guarantees... @pellared wrote #3919 (comment):
Well, I guess I could be an outlier, but I have this suspicion that if you could peer into the private repositories of the world, you'd probably find more than a few examples of this in the wild: // addLinks is a workaround to add links to an existing span pending https://github.com/open-telemetry/opentelemetry-go/issues/3919
func addLinks(ctx context.Context, links ...trace.Link) {
ctx, span := trace.SpanFromContext(ctx).TracerProvider().Tracer(scopeName).Start(ctx, "add_links", trace.WithLinks(links...))
defer span.End()
}
// addLinks is a workaround to add links to an existing span pending https://github.com/open-telemetry/opentelemetry-go/issues/3919
func addLinks(ctx context.Context, links ...trace.Link) {
if s, ok := trace.SpanFromContext(ctx).(interface{ addLink(link trace.Link) }); ok {
for _, link := range links {
s.addLink(link)
}
}
} |
I doubt that we will have a consensus to define an experimental feature using anonymous type assertions. Personally, I am rather against this idea. However, I do my best to be open-minded. It will be hard for users (and even us - maintainers as well) to "track" (and document) the evolution of experimental features which are used in a "duck-typing" (technically: "structural typing") way. EDIT: I edited the issue description to make it clear that it is currently blocked until it is stable in the specification. Feel free to join the SIG meeting if you want to discuss it.
This will not work. See: https://go.dev/play/p/aoXktbLMty9 |
Looks like I have a few batching scenarios where I'd like one trace for the batch, and then separate trace for each item in the batch. |
See #5032 Closing, as AddLink has been added and will be usable in the next release. |
Blocked by
Problem Statement
We need to add the Method AddLink() to the span interface. This was the decided route at the end of the 2023/03/21 Sepcification Meeting (youtube link eventually).
How do we add this does have an impact on application writers using this API/SDK. I have written a quick explanation of how three different approaches affect users: https://github.com/MadVikingGod/intexplore/blob/main/readme.md.
The crux of the problem is that when we do this, SDK V1.n.0 won't be compatible with API V1.n+1.0. Because the "client", the application writer, chooses the SDK directly, but the API both directly and indirectly through the minimum version used their dependencies, this is a situation that users will eventually face.
Proposed Solution
The current solution agreed to was to add the method to the interface. This is represented by
direct
in the above example.Alternatives
Use a different approach; two others that are also detailed are:
Another alternative that could be considered is if we have a fully functional public NoOp implementation in the API package that could be embedded. This has been discussed but rejected because A) this exposes a different way to acquire a span/tracer/tracerProvider, and B) this will have silent dropping behavior.
Additional Context
This issue is to formalize our understanding of how to proceed if/when AddLink is added.
The text was updated successfully, but these errors were encountered: