Is the following Streaming implementation follows MetricExporter and Producer specifications?

[asafm]

Hi,

I'm working on replacing several metrics libraries with OpenTelemetry Metrics in Apache Pulsar. Since it is a very latency-sensitive system, a lot of effort went into minimizing memory allocations where possible. I've read OTel Metrics Java SDK code, and it seems that in certain conditions (i.e., 100k topics and above) the amount of memory allocations cost would be severe.

Hence, I've submitted a proposal to enhance the SDK such that it will use a different data structure for relaying the metric points - i.e. using the concept of streaming instead of batching in memory (list) thus minimizing memory allocations to bare minimum. I'll describe in short the bare-bones suggestion here, and you can read it more thoroughly in the GitHub issue, including the context of Apache Pulsar in more detail.

We'll add two new interfaces:

MetricsVisitor {
	resourceStarted(Resource)
	instrumentationScopeStarted(IntrumentationScopeInfo)
	instrumentStarted(MutableInstrumentInfo)
	visitAttributes(MutableMetricData)		
	instrumentEnded()
	instrumentationScopeEnded()
	resourceEnded()	
}	

MetricsBatch {
	visit(MetricsVisitor)
}

We'll use them to have the exporter support streaming (and batching), and upon creation you choose the export method.

MetricsExporter {
    // existing
    CompletableResultCode export(Collection<MetricData> metrics);

    // new
    ExportMode getMode() {
        return BATCH; // for backward compatability
    }
    
    // new
    ExportResult export(MetricsBatch)
}

The MetricsProducer will be modified to support that:

MetricsProducer
    // existing (slightly renamed as it is still internal package)
    Collection<MetricData> collectAllMetricsAsCollection();
        // Default implementation will be to call collectAllMetrics and batch it into a collection

    // new
    MetricsBatch collectAllMetrics()

The specification for MetricsExporter.export(batch) says:

batch - a batch of Metric points. The exact data type of the batch is language specific, typically it is some kind of list. The exact type of Metric point is language specific, and is typically optimized for high performance.

The main idea here is that we use a different data structure to produce and export those batch of metrics.

The benefit and goal (outlined in detail in the original proposal) is to reduce any overhead cost in terms of CPU and memory the OTel Java SDK bears on the host application. I believe this should be the goal of any SDK.

My question is:
Does adding this ability to receive the batch as streaming (different data structure) follows the specifications and allowed?

[asafm]

@jsuereth Would love to hear your take on this and help me figure how to proceed to get a definitive answer on this and next steps if needed

[reyang]

For example, in OpenTelemetry .NET there is an abstraction called Batch and it can be a real list/array or something that doesn't physically exist.

• https://github.com/open-telemetry/opentelemetry-dotnet/blob/3a491b14b24c2560e27ff0b74f86359e9e0d4063/src/OpenTelemetry/Metrics/BaseExportingMetricReader.cs#L87
• https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry/Batch.cs

[asafm]

@jsuereth

I'm not sure we want to blend the two MetricProducer types into one. I think if an SDK wishes to abstract that detail, great. However we're talking about different integrations with different performance characteristics (like MetricReader vs. MetricExporter). I personally would prefer them to be explicit.

Implementation wise, you're definitely correct, and I believe it should be - under the hood explicit.
I think user experience should be key.
Let's assume you have both implementations for the SDK producer (batch, streaming) and also for OTLP exporter (both come with the SDK).
If it is explicit, the user have a choice when registering the exporter, right?

otlpExporter = new OtlpStreamingExporter() // or OtlpBatchExporter
reader.registerExporter(otlpPExporter)

I think for the end user, it is confusing. From their perspective, they should always prefer the less costly (memory, CPU) implementation, hence the Streaming one.

If we were to have it as one interface, with two modes, then we look at the same code, modified:

otlpExporter = new OtlpExporter() 
reader.registerExporter(otlpPExporter)

The reader will have the SdkExporter registered (internally by the SDK), which supports streaming mode, and OtlpExporter will support streaming mode, thus it will choose streaming both ways to minimize memory consumption.
I think it creates a more friendly and simple user experience this way.
Under the hood, OtlpExporter can be divided into two components implementing the two ways, and it will simply instantiate and call the appropriate one.

I think this API should be completely optional for SDKs to provide. It's a great performance optimistion, but is also expensive to implement and may not provide as much value in some languages. I think this should allow Java SDK to include what you need.

I agree that it shouldn't be mandatory, yet should advise SDK to follow that path, since it is the goal of OTel SDK to be as lean as possible in terms of memory (common sense, and also based on Memory Management section in the specifications) and streaming is the leanest way.

[asafm]

@reyang

It sounds to me like an implementation question instead of specification question.

So if I understand you correctly, your answer to this is: you can use the API you wrote above as it conforms with the spec, since the spec doesn't strict you to use a very specific data type for returning that metric data.

[reyang]

@reyang

It sounds to me like an implementation question instead of specification question.
So if I understand you correctly, your answer to this is: you can use the API you wrote above as it conforms with the spec, since the spec doesn't strict you to use a very specific data type for returning that metric data.

Yes.

[asafm]

@jsuereth Perhaps you can help me here process wise: So far I have one person's answer to my question.
How many people or which people need to answer this question, so I can receive an approval to continue with implementing open-telemetry/opentelemetry-java#5105 (knowing it aligns with the spec)?

[jmacd]

I would like to ask whether there are API-level changes that can help with the performance issues directly as to accompany the above answer about optional SDK-implementation features.

In early drafts of OTel we discussed "bound" metric instruments, those which allow the user to pre-process a large part of the metrics operation cost and avoid repeating that work. Can an instrument.Bind(attributes...) operation help avoid the need for new SDK optimizations?

Likewise, can a batch metrics update API help? If the operation was on a single attribute set and a batch of instrument values, would that help reduce allocations in your application? These two ideas can be complimentary.

I could imagine that with the above-described API extensions, the SDK could be optimized to avoid the costs you're trying to avoid.

[asafm]

@jmacd Can you please elaborate what exactly is the bind operation of attributes to instrument and how it lowers memory footprint / memory allocations ?

Likewise, can a batch metrics update API help? If the operation was on a single attribute set and a batch of instrument values, would that help reduce allocations in your application? These two ideas can be complimentary.

From what I've seen, usually updates (like inc/record) just update in-place variables, so no extra memory footprint or memory allocation. The cost today lies in the collection path(): the SDK metrics producer, allocates a new object to return the snapshot of the current aggregate value of the instrument(/view) for a specific attribute. The idea in my proposal was to reverse the direction: instead of me giving you a list of data, I iterate over the values and executes a function to let you know: visitInstrument, visitInstAttr, etc. This way the only memory allocation is a single data-transfer object for each type (explicit bucket, counter, etc).

[jack-berg]

@asafm is correct. The java metric SDK allocation free on the record path one you reach steady state, assuming you've pre-allocated your Attributes ahead of time. As of this point in time, the only thing bound instruments would get you is avoiding having to do do a constant time lookup in a map to find the aggregator corresponding to Attributes for a measurement. IMO, the performance benefit of saving this map lookup does not warrant the increased API surface area and reduced ergonomics of bound instruments.

I've been making a bunch of changes to the java metrics SDK to reduce allocations on the collect path. At this point, the only allocations remaining are to produce the immutable MetricData records passed to MetricExporter#export(Collection). Even these allocations can be avoided if we were to adopt a strategy of reusing MetricData points as I've prototyped here. This is my preferred solution to the problem because:

1. Adding a visitor pattern for MetricExporter would be inconsistent with exporters for other signals (SpanExporter, LogRecordExporter).
2. The visitor pattern complicates the pattern chaining together multiple exporters and delegating from one to another.