Allow exporters to decorate SpanData

[anuraaga]

Is your feature request related to a problem? Please describe.

Exporters input SpanData and export to a backend. It seems reasonable to allow an exporter to do backend-specific transformations on the SpanData during this process. Currently, there is no simple way of doing this, trying to create the object requires internal or testing classes like SpanWrapper or TestSpanData.

Describe the solution you'd like

Provide a SpanDataWrapper abstract class that delegates to a SpanData. Users can override this and implement methods they want to augment when decorating.

Describe alternatives you've considered

SpanData.toBuilder to allow a user to then modify and build a new SpanData. It works well too but probably has more overhead.

Additional context
Add any other context or screenshots about the feature request here.

[jkwatson]

Hi @anuraaga . I'm confused with what you need here. Currently, an Exporter can do whatever it wants with the data that it is given. Why would you need to modify the SpanData itself before putting it on the wire? I feel like I'm missing something.

[anuraaga]

@jkwatson Ah I missed some important context. The idea is to use an official exporter, such as the OTLP exporter, delegating to it after tweaking the spans. So for example, when the java auto instrumentation was marking AWS SDK spans as INTERNAL, the exporter was rewriting them to be CLIENT and delegating away.

Admittedly, this is a lot of back-and-forth with auto instrumentation, as I'll find some behavior that doesn't work well for us and tweak it in the exporter, but may be able to upstream the behavior into the instrumentation when it makes sense. I sort of expect this won't always be the case and will need to decorate in production too. For example, setting db.instance for redis works better for our data model but isn't necessarily a general concept.

This actually relates to a gitter post I made a while ago about span customization

https://gitter.im/open-telemetry/opentelemetry-java?at=5ec624726e837c7743c3f6ac

The impression I got is that the exporter is the right place to apply such customization, but if there was a separate API dedicated to customization, possibly given a mutable span, that would work well too and would more directly solve the problem.

[jkwatson]

Hm. It seems like if the auto-instrumentation isn't doing the right thing, that we should fix that, rather than complicate the export pipeline.

If you have a specific non-standard need, would it make sense to have this sort of customization possible in the collector, rather than having to embed it in every processes exporter pipeline?

[anuraaga]

I haven't closed this yet since I've always felt like this is important, but didn't have a clear example at the time. This might be one though

open-telemetry/opentelemetry-java-instrumentation#555

Especially for auto instrumentation, we'd generally need to be able to map an endpoint to a service name or many spans will not be easily distinguishable from each other, e.g. for separate redis clients (though I have met people that memorize all the IP addresses in the system XD). While it's possible to hook this into span creation itself, this seems like a global processing step that could use a more formal hook IMO. Decorating on export would be OK and requires no new API, just that currently it's tedious so a helper like SpanDataWrapper can, well help. But being provided a mutable Span itself in a separate API is actually closer to what I'm after conceptually I think.

I don't think this sort of app-level customization will always work well in a declarative configuration in the otel collector (besides that requiring the collector in the first place) while also keeping concerns local - it is the app that is concerned about a particular endpoint and customization based on this is simple in an app, while the collector would need to support all endpoints of all apps it supports, for example.

[jkwatson]

I'm definitely not opposed to having this happen, but I think it's important enough that it warrants a proposal in the specs. I would recommend the following course of action: write a spec issue, propose a spec update to support this, and submit a PR that would demonstrate how it would work in Java. Does that sound like something you could take on, @anuraaga ?

[anuraaga]

Yup filed open-telemetry/opentelemetry-specification#663 first to discuss the spec

[arminru]

Hi @anuraaga! Just to clarify, you are rather thinking about span processors here, right?
Exporters are on the far end of the pipeline anyway and can do whatever they want to the data they export. Altering the actual span at that point wouldn't have any effect anyway (except for potential race conditions). So if you would want to intercept spans globally before they are handed to any exporters, span processors would be the way to go here.

[anuraaga]

Yeah I think span processors make sense for this. I mention exporter in this issue because when I asked on Gitter a while ago it seemed like people were more for sticking with doing customization in the exporter using decoration. Or maybe I just interpreted that way since it did indeed work just tedious :) But your comment on the spec issue makes me lean even more toward solving this with span processors.

[iNikem]

I would like to bridge this to open-telemetry/opentelemetry-java-instrumentation#566

One of the requirements for vendor-specific distribution that we want to support is the ability to add new attributes to spans. Sometimes the values of those attributes may depend on other attributes. Which means that there should be some place in span processing pipeline, where span/spandata/whatever should be both readable and writable.

Terminal exporter is a bad place for this logic. Vendors don't want to replace exporting protocol, they just want to add attributes. Meaning, to add a function Span->Span. Currently there is no place in SDK where such function could be added.

As open-telemetry/opentelemetry-java-instrumentation#566 is required for GA, I propose to relabel this issue correspondingly.

@jkwatson @anuraaga @trask

[carlosalberto]

@iNikem You mean, rename this issue to Allow exporters... etc?

[iNikem]

@carlosalberto Yes. And make it release:required-for-ga

[carlosalberto]

@iNikem Makes sense.

@anuraaga Any objection to rename/edit this issue to make it SpanProcessor oriented?

[iNikem]

@carlosalberto SpanExporter, not SpanProcessor

[carlosalberto]

Terminal exporter is a bad place for this logic.

I'm confused now. You want to have this decoration logic where? SpanProcessor or SpanExporter? 😉

[iNikem]

SpanExporter. This issue should be SpanExporter oriented, to allow SpanExporter to decorate SpanData.

[anuraaga]

To be honest, I don't care where the decoration is as long as it's possible ;) My gut instinct still says SpanProcessor, probably with another lifecycle method for decoration pre-export, is cleaner. But there seems to be more buy-in on making this a part of the exporter and I'd rather stick with that momentum so this issue title seems fine as is to me.

[jkwatson]

Since we have the public SpanData interface, nothing is stopping you from creating your own wrapper and using it in a delegating exporter. Do we truly need to have the wrapper as a part of the SDK to enable your use case? [I think that SpanData was a final class when you initially created this issue, so I'm wondering if this is now something that can just be done by the exporter author as needed, now that it's an interface].

[anuraaga]

I think there are two issues with requiring users to write their own wrapper

• It's pretty tedious to write the wrapper since there are so many methods - it's not very good UX
• It's less safe for users since they'll have to update the wrapper any time the SpanData interface changes, even if the change is supposed to be backwards compatible. Though this does make me wonder if that's an issue with the SDK in general, will we never add methods to Span.java to ensure custom implementations don't break? Maybe we need to consider using abstract classes instead of interfaces for the larger APIs to have the option of empty implementations.

[jkwatson]

Sure would be nice if we could use java 8 with default methods, eh?

[jkwatson]

So, all it would take to close this issue would be a SpanDataWrapper or DelegatingSpanData class that allowed easy extension to override any of the SpanData methods? I don't have any opposition to that being added.

[iNikem]

I don't understand why delegation is the best way to augment existing SpanData with new information. I wanted to propose something like SpanData.toBuilder(), which I then can use to add/change data and then call build to get new SpanData.

[anuraaga]

@iNikem The only reason is performance, can be expensive to copy into builder and back. But I agree it's probably an easier to use API, I'm ok with either approach really.

[iNikem]

I still don't understand how delegating would work :) Suppose we do DelegatingSpanData. We want to allow adding new attributes/events/links. SpanData.getAttributes returns ReadableAttributes where you cannot add new attributes. So how will DelegatingSpanData.getAttributes return ReadableAttributes with both old and new attributes?

[anuraaga]

It probably requires Attributes.toBuilder :) TBH these concepts are vaguely in my head and I want to try them out - I can get something ready by beginning of next week so if anyone wants, feel free to assign this to me.

[carlosalberto]

I wanted to propose something like SpanData.toBuilder(), which I then can use to add/change data and then call build to get new SpanData

+1 for this approach.

[anuraaga]

I know we've been lukewarm about the need for decoration, @jkwatson @iNikem but does open-telemetry/opentelemetry-specification#823 provide more of a need?

[iNikem]

I think we need some easy way to wrap span exporters for SpanData decoration. We just could not agree on the implementation details :)

[jkwatson]

Yeah, I don't disagree with the need. I'd prefer using the delegate option vs. using a builder, just from an allocation perspective. Having a delegate also doesn't mean we can't add a builder option later (or vice-versa).

I thought that @iNikem 's main complaint, though, was the challenge in dealing with the immutable ReadableAttributes interface, rather than the specifics of how precisely to apply the decoration. Don't you have the same issues with having to read and modify the attributes with either solution?

Would it make sense to add a method to the Attributes class that would provide an Attributes.Builder based on the contents of a ReadableAttributes instance? Something like a static Attributes.toBuilder(ReadableAttributes attributes) ?

[iNikem]

Correct, I prefer to avoid writing a wrapper for SpanData but I can do that. Having attributes builder probably will benefit me more than builtin wrapper.

[jkwatson]

ok, let's create the builder method, as mentioned. what would you say to having this in some kind of sdk extension module, where we can incubate SDK additions before adding them to the official SDK? We could actually put both options into that module for now, if desired.

[iNikem]

I have not problems with extension modules

[jkwatson]

@anuraaga Could you take your two option PRs, and bundle them up into one new PR that adds an sdk-incubator extension module for them? Meanwhile, I'll create a separate issue for the new builder method.

[jkwatson]

#1553

[anuraaga]

Thanks yeah will work on an extension module.

[jkwatson]

@anuraaga any update on this?

[anuraaga]

Sorry for the delay, still haven't gotten around to this, will prioritize for early next week.

[jkwatson]

I think this is done, or at least do-able with #1625 . Can we close this now?

[anuraaga]

I'm torn on whether we close this issue with just the incubator or not. Either's fine with me

[jkwatson]

closing this. Moving the features into the main API can be tracked with separate issues/PRs, and if we need a more robust exporter API, that should start with the specs.