Performance: send resource data to span processors once, not for every span when it ends

[shaun-cox]

sdk::trace::Span::ensure_ended_and_exported clones the provider's config resource for every span processor and every span before sending it to the span processor.

But since the resource is unique to the provider which owns all the span processors, this data could be sent ahead of time, just once, to each span processor.

Not only would this reduce CPU consumption by not cloning, but it will also reduce the size of each message sent through a span processor channel by about 64 bytes (estimated) which will save CPU all by itself.

(I can work on a PR for this but wanted to hear of any objections here before starting that work.)

[cijothomas]

+1
The same idea could be used in other signals as well (logs, metrics).

[lalitb]

I may be missing something basic here, but can't we have build_export_data take reference to the Resource object, just like it does for Tracer ? Probably I am thinking C++ way, and rust lifetime scoping makes it difficult to pass reference here :)

[djc]

By "sent", do you mean it would just be passed at initialization? Or do we need some other (more clever) path to pass it?

Anyway, this sounds like it could make a lot of sense.

[shaun-cox]

I may be missing something basic here, but can't we have build_export_data take reference to the Resource object, just like it does for Tracer ? Probably I am thinking C++ way, and rust lifetime scoping makes it difficult to pass reference here :)

It could take a reference, but it would still need to invoke .clone() through that reference because its job is to return an export SpanData instance, which owns its Resource. You can kinda see the fundamental issue there... we're bringing SpanContext, Resource, and InstrumentationLib together with the core SpanData and combining them all to build a new export SpanData) to pass to the processor. But the processor may see that the SpanContext is not being sampled and do nothing... making all the work we just did to clone/build the export SpanData be for nothing... Further, only the SpanContext and core SpanData are unique to the current span being processed... the Resource (and InstrumentationLib) are not and could be "staged" in the exporter to be married with this unique span data on the export thread, not on this instrumentation thread.

By "sent", do you mean it would just be passed at initialization?

At provider initialization, yes, but not technically at span processor initialization, as that happens while sdk::trace::provider::Builder is accumulating span processors. Once build is invoked to create the TraceProvider, we'd have to invoke a new method on SpanProcessor to pass the configuration's Resource. The SpanProcessor implementations will then "send" it to their exporter counter-part on the receive end of the channel separating processor from exporter.

(Hope that makes more sense. In hindsight, the title of this issue should have said "send resource data to exporter once", or "send resource data through span processor to exporter once".)

[djc]

At provider initialization, yes, but not technically at span processor initialization, as that happens while sdk::trace::provider::Builder is accumulating span processors. Once build is invoked to create the TraceProvider, we'd have to invoke a new method on SpanProcessor to pass the configuration's Resource. The SpanProcessor implementations will then "send" it to their exporter counter-part on the receive end of the channel separating processor from exporter.

Uhh, sounds like some type system shenanigans might be necessary. I guess I don't really understand why we separate the processor and the exporter anyway.

(Hope that makes more sense. In hindsight, the title of this issue should have said "send resource data to exporter once", or "send resource data through span processor to exporter once".)

You can still change it. 😛

[shaun-cox]

First, I'm going to use the following to disambiguate exported SpanData from core SpanData:

• ExportedSpanData
• SpanData

Notice that ExportedSpanData logically equals (SpanContext, SpanData, Resource, InstrumentationLibrary).

Today we have this trait:

pub trait SpanProcessor {
    ...
    fn on_end(&self, span: ExportedSpanData);
    ...

There are two issues with this interface:

1. it requires the full exported span data be built in the first place, when it's the processor's job to decide if it should be exported at all.
2. even if the processor decides it should be exported, the means to do so more efficiently are removed.

I suspect a better interface might be something like:

pub trait SpanProcessor {
    ...
    fn on_end(&self, ctx: &SpanContext, data: &SpanData, lib_id: LibId);

    fn set_resource(&self, resource: Resource);
    fn set_libraray(&self, id: LibId, library: InstrumentationLibrary);
    ...

Now on_end takes borrowed references to immutable data (or scalar primitives), not large, owned data, and the processor can send much smaller messages to the exporter for each span it decides to export. It's now pushed down to the exporter to marry up the context and core span data with the fixed resource (and InstrumentationLibrary) attributes in the export thread.

I guess I don't really understand why we separate the processor and the exporter anyway.

It's to be able to do batch processing (in the exporter) and provide the instrumented code a bit of insulation from however the sdk is configured (or not) to "deal with" (i.e. process and export) spans. The processor(s) are, if configured, dynamically dispatched to on the thread of whatever is being instrumented. Their job is chiefly to make an export decision and ideally not have the instrumented code feel the cost of exporting directly... this practically means they are going to send their decision over a channel to the exporter to deal with the heavy lifting on its own thread.

[djc]

It's to be able to do batch processing (in the exporter) and provide the instrumented code a bit of insulation from however the sdk is configured (or not) to "deal with" (i.e. process and export) spans. The processor(s) are, if configured, dynamically dispatched to on the thread of whatever is being instrumented. Their job is chiefly to make an export decision and ideally not have the instrumented code feel the cost of exporting directly... this practically means they are going to send their decision over a channel to the exporter to deal with the heavy lifting on its own thread.

Ah, right, so the processor is sort of "synchronous" with respect to user code, while the exporter is "asynchronous". So when using tracing-opentelemetry, is there still a processor being invoked like this by the tracing code code?

(Your proposed new interface makes a lot of sense to me.)

[shaun-cox]

Ah, right, so the processor is sort of "synchronous" with respect to user code, while the exporter is "asynchronous".

Exactly.

So when using tracing-opentelemetry, is there still a processor being invoked like this by the tracing code code?

Yes. Depending how you've configured your otel pipeline, you likely have at least one processor active (Simple or Batched). tracing-opentelemetry basically installs another tracing Layer and accumulates into an otel SpanBuilder while their span is active. When their (i.e. tracing crate) span is closed, our builder is used to start and export our span. (So, unfortunately, it's their cost plus our cost, I've learned. So any optimizations we make here, will improve the use cases of tracing-opentelemetry too.)

[cijothomas]

Ah, right, so the processor is sort of "synchronous" with respect to user code, while the exporter is "asynchronous".

Exactly.

Just to add that the above is true majority of the time. (when using any of the exporters from OTel spec like OTLP, Zipkin etc.). There are exporters to OS_native tracing systems, like https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-user-events-logs/src/logs/reentrant_logprocessor.rs, where exporter and processors are synchronous with respect to user code!

[TommyCpp]

Yeah, I agree we shouldn't transfer Resource as owned value in SpanProcessor.

But I don't think SpanProcessor needs set_resource function. Since the Resource is used to identify the entity that emits the telemetry. It should only store in TracerProvider. Thus, we can just have a reference from SpanData to TracerProvider and Tracer that allow the exporter downstream to access Resource and InstrumentationLibrary information

[shaun-cox]

Thus, we can just have a reference from SpanData to TracerProvider and Tracer that allow the exporter downstream to access Resource and InstrumentationLibrary information

While this is true, just want to point out that the processor(s) communicate with their exporter(s) asynchronously over a channel by sending them an ExportedSpanData message. That message will need to now contain TracerProvider and Tracer based on above suggestion. TracerProvider contains one Arc (to provider), and Tracer contains two more (one to instrumentation library and another to same provider). These counts would be atomically incremented before insertion to the channel and then decremented when consumed by the exporter thread. Hence, those cache lines containing the reference counts are hotter and bouncing back and forth between the cpus running the threads interacting with the channel.

But the TracerProvider owns the span processors (which own their exporter) anyhow... so there is no need to bump the reference count on the provider in order to keep it alive on a per-processed-span basis. It's sufficient to send it only once... hence the proposed new trait methods.