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.

Sign up for GitHub

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

Jump to bottom

problems with generated exponential histogram #3767

Open
loomis-relativity opened this issue Mar 8, 2024 · 1 comment · May be fixed by #3771
Open

problems with generated exponential histogram #3767

loomis-relativity opened this issue Mar 8, 2024 · 1 comment · May be fixed by #3771
Labels
bug Something isn't working

Comments

@loomis-relativity
Copy link

loomis-relativity commented Mar 8, 2024 •
edited

Describe your environment

Running with:

  • MacBook Pro Intel
  • MacOS 14.3.1 (intel)
  • Python 3.11.7 (homebrew)

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 loomis-relativity added the bug Something isn't working label Mar 8, 2024
@loomis-relativity
Copy link
Author

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.

@loomis-relativity loomis-relativity linked a pull request Mar 11, 2024 that will close this issue
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

remove unnecessary buckets on exponential histograms loomis-relativity/opentelemetry-python
1 participant
@loomis-relativity