opentelemetry-go
opentelemetry-go copied to clipboard
open-telemetry
Feature: AddLinks() to spans.
Blocked by
- https://github.com/open-telemetry/opentelemetry-specification/issues/3865
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:
- Embedding the interface in the SDK struct. (abstract in the example)
- Use a secondary interface and have the callers of that API assert. (side in the example)
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.
Should the blocked label be removed following resolution in open-telemetry/opentelemetry-specification#454?
For clarity, it seems to be blocked on
AddLinkstill being marked experimental here
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 AddLinks() is one of those tough cases where the work around (e.g. creating dummy spans just to capture links) feels significantly worse than opting into using an API surface that's documented as experimental and known to be subject to breaking changes.
Context that's mostly irrelevant, but might help shed some light on the motivations for this^ question:
I need to add a link to a span for which I don't control the creation (e.g. lambda). Creating a second span just to add a link feels wasteful (and, to boot, looks silly in tracing UIs).
The details of why I have to do this are (IMO) interesting, but the main point is that this pattern is something that comes up in a number of scenarios, and other language SDKs (e.g. .NET) already support it, so I've got developers asking why we have to use a different pattern in golang.
If the answer is just that there's no contributor bandwidth for implementing experimental stuff, I'd be happy to contribute to that effort, but if it's a matter of maintainer bandwidth and needing to focus efforts, I would also understand, so just looking for some context on how the maintainers are thinking about these features and the blocked label generally.
One problem is to make it "experimental" in SDK, the other is to make it available for API users.
Maybe under an isolated module that could clearly/cleanly document that functions in such a module are subject to breaking changes?
Can you propose a design?
One problem is to make it "experimental" in SDK, the other is to make it available for API users.
I may well be underthinking the implications, but if AddLinks() was only available in the SDK, I think that might be enough for my use-case? I'm making a crummy trade-off today, so depending on the SDK where I should only need the API feels potentially less crummy? Idk, will think more about this.
Maybe under an isolated module that could clearly/cleanly document that functions in such a module are subject to breaking changes?
Can you propose a design?
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(...) {
...
}
@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 https://github.com/open-telemetry/opentelemetry-go/issues/3919#issuecomment-1930874801:
@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.
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 https://github.com/open-telemetry/opentelemetry-go/issues/3919#issuecomment-1930890591:
On the other side, I do not see a lot of demand for this feature. Therefore, I will rather wait until it is stable in the OTel Specification.
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()
}
~~though, based on your earlier snippet, I'm now thinking I might be able to hack at it in a different (better?) way:~~ edit: nope, you're right, wrote a bad scratch test, oops
// 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.
I'm now thinking I might be able to hack at it in a different (better?) way:
This will not work. See: https://go.dev/play/p/aoXktbLMty9
I created https://github.com/open-telemetry/opentelemetry-specification/issues/3865
Looks like AddLink is no longer experimental now that https://github.com/open-telemetry/opentelemetry-specification/pull/3887 went through.
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. AddLink would help me make a bidirectional links between those two so I can navigate easier via the tempo UI.
See https://github.com/open-telemetry/opentelemetry-go/pull/5032
Closing, as AddLink has been added and will be usable in the next release.
