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

Feature: AddLinks() to spans. #3919

Closed
MadVikingGod opened this issue Mar 22, 2023 · 16 comments
Closed

Feature: AddLinks() to spans. #3919

MadVikingGod opened this issue Mar 22, 2023 · 16 comments
Labels
area:trace Part of OpenTelemetry tracing blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made enhancement New feature or request pkg:API Related to an API package

Comments

@MadVikingGod
Copy link
Contributor

MadVikingGod commented Mar 22, 2023

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:

  • 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.

@MadVikingGod MadVikingGod added enhancement New feature or request pkg:API Related to an API package area:trace Part of OpenTelemetry tracing blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made labels Mar 22, 2023
@austindrenski
Copy link

Should the blocked label be removed following resolution in open-telemetry/opentelemetry-specification#454?

@abh
Copy link

abh commented Jan 27, 2024

Related from when this was removed some years ago: #329, #349

@MrAlias
Copy link
Contributor

MrAlias commented Jan 29, 2024

Blocked on the specification stabilizing this feature.

@hongalex
Copy link

hongalex commented Feb 6, 2024

For clarity, it seems to be blocked on AddLink still being marked experimental here

@austindrenski
Copy link

austindrenski commented Feb 6, 2024

For clarity, it seems to be blocked on AddLink still 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.

@pellared
Copy link
Member

pellared commented Feb 6, 2024

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?

@austindrenski
Copy link

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(...) {
	...
}

austindrenski added a commit to austindrenski/opentelemetry-go that referenced this issue Feb 6, 2024
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>
austindrenski added a commit to austindrenski/opentelemetry-go that referenced this issue Feb 6, 2024
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>
@austindrenski
Copy link

austindrenski commented Feb 6, 2024

@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...)
}

@MrAlias
Copy link
Contributor

MrAlias commented Feb 6, 2024

@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.

@pellared
Copy link
Member

pellared commented Feb 6, 2024

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.

@pellared
Copy link
Member

pellared commented Feb 6, 2024

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.

@austindrenski
Copy link

austindrenski commented Feb 6, 2024

@MrAlias wrote #3919 (comment):

@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 #3919 (comment):

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)
		}
	}
}

@pellared
Copy link
Member

pellared commented Feb 6, 2024

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

@pellared
Copy link
Member

pellared commented Feb 7, 2024

I created open-telemetry/opentelemetry-specification#3865

@ryepup
Copy link

ryepup commented Apr 3, 2024

Looks like AddLink is no longer experimental now that open-telemetry/opentelemetry-specification#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.

@dmathieu
Copy link
Member

dmathieu commented Apr 3, 2024

See #5032

Closing, as AddLink has been added and will be usable in the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:trace Part of OpenTelemetry tracing blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made enhancement New feature or request pkg:API Related to an API package
Projects
None yet
Development

No branches or pull requests

8 participants