open-telemetry / opentelemetry-python

OpenTelemetry Python API and SDK
https://opentelemetry.io
Apache License 2.0
1.76k stars 615 forks source link

problems with generated exponential histogram #3767

Open loomis-relativity opened 7 months ago

loomis-relativity commented 7 months ago

Describe your environment

Running with:

Requirements:

opentelemetry-api>=1.23.0
opentelemetry-sdk>=1.23.0
opentelemetry-proto>=1.23.0
opentelemetry-exporter-otlp>=1.23.0

Steps to reproduce

Run main.py (below) and capture the dump of the exponential histogram. Source code:

from opentelemetry import metrics
from opentelemetry.exporter.otlp.proto.http.metric_exporter import OTLPMetricExporter
from opentelemetry.sdk.metrics import MeterProvider
from opentelemetry.sdk.metrics.export import PeriodicExportingMetricReader

def main():

    exporter = OTLPMetricExporter()
    meter_provider = MeterProvider([PeriodicExportingMetricReader(exporter)])
    meter = metrics.get_meter(
        name="test-histogram",
        meter_provider=meter_provider,
    )
    histo = meter.create_histogram("test-histogram")
    for i in range(1000):
        for _ in range(i):
            histo.record(i, {"type": "test-histogram"})

    meter_provider.shutdown()

if __name__ == "__main__":
    main()

This is run with the following script:

export OTEL_EXPORTER_OTLP_ENDPOINT=http://localhost:4318
export OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE=DELTA
export OTEL_EXPORTER_OTLP_METRICS_DEFAULT_HISTOGRAM_AGGREGATION=base2_exponential_bucket_histogram

python3 ./main.py

#

What is the expected behavior?

I expected to see a dump like that generated for the JavaScript SDK in dump-js.txt.

What is the actual behavior?

The generated exponential histogram in dump-python.txt seems to have the following problems:

  1. There is a negative bucket, even though no negative values are added to the histogram. (This actually causes a downstream vendor to reject the histogram.)
  2. All 160 buckets are provided even though the last 79 are all empty. Other SDKs (e.g. Javascript, dump-js.txt) trim unnecessary buckets, reducing the size of the payload.

Additional context

None.

loomis-relativity commented 6 months ago

One naive option would be to modify this block of code in the exporter:

                        if data_point.positive.bucket_counts:
                            buckets = nonzero_slice(data_point.positive.bucket_counts)
                            if buckets:
                                positive = pb2.ExponentialHistogramDataPoint.Buckets(
                                    offset=data_point.positive.offset,
                                    bucket_counts=buckets,
                                )
                            else:
                                positive = None
                        else:
                            positive = None

                        if data_point.negative.bucket_counts:
                            buckets = nonzero_slice(data_point.negative.bucket_counts)
                            if buckets:
                                negative = pb2.ExponentialHistogramDataPoint.Buckets(
                                    offset=data_point.negative.offset,
                                    bucket_counts=buckets,
                                )
                            else:
                                negative = None
                        else:
                            negative = None

also adding the function:

def nonzero_slice(lst):
    for i, value in enumerate(reversed(lst)):
        if value != 0:
            return lst[0:len(lst)-i-1]
    return []

This produces a dump file (dump-corrected-python.txt) that has no negative buckets and the positive buckets include only the lowest, non-zero buckets.

There is some overhead in searching for the last non-zero bucket, but it does produce a more compact representation of the payload (which also works with the downstream vendor). If this would be acceptable to the maintainers, I'm happy to code up a PR with this change.