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.

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

Fix OTLPMetricExporter ignores preferred_aggregation property #3603

Merged

Conversation

Dudssource
Copy link
Contributor

@Dudssource Dudssource commented Dec 27, 2023

Description

OTLPMetricExporter was ignoring the preferred_aggregation property, not allowing any kind of customization directly on the constructor.

Fixes #3522

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Testing informing a custom aggregation (like informing an ExplicitBucketHistogramAggregation)

I've tried the following configuration for my MetricReader:

# OTLP metrics
reader = PeriodicExportingMetricReader(
    OTLPMetricExporter(
        endpoint="http://localhost:4318/v1/metrics",
        session=session,
        compression=Compression.Gzip,
        timeout=5,
        preferred_aggregation={
            Histogram: ExplicitBucketHistogramAggregation(
                boundaries=[0.05, 0.1, 0.5, 1, 1.5, 2, 2.5, 3, 4, 5, 10, 20, 30, 40, 50, 60, 300, 600],
            )
        },
    )
)

On my OTLP collector (localhost:4318), I configured the logging exporter for metrics:

receivers:
  otlp:
    protocols:
      http:
        endpoint: 0.0.0.0:4318

exporters:
  logging:
    verbosity: detailed

service:
  telemetry:
    logs:
      level: DEBUG
  pipelines:
    metrics:
      receivers: [otlp]
      processors: []
      exporters: [logging]

After this setup, I was able to see my histograms being exported with my explicit configured boundaries.

Before (without the fix):

otel-collector_1  | Count: 1
otel-collector_1  | Sum: 0.808466
otel-collector_1  | Min: 0.808466
otel-collector_1  | Max: 0.808466
otel-collector_1  | ExplicitBounds #0: 0.000000
otel-collector_1  | ExplicitBounds #1: 5.000000
otel-collector_1  | ExplicitBounds #2: 10.000000
otel-collector_1  | ExplicitBounds #3: 25.000000
otel-collector_1  | ExplicitBounds #4: 50.000000
otel-collector_1  | ExplicitBounds #5: 75.000000
otel-collector_1  | ExplicitBounds #6: 100.000000
otel-collector_1  | ExplicitBounds #7: 250.000000
otel-collector_1  | ExplicitBounds #8: 500.000000
otel-collector_1  | ExplicitBounds #9: 750.000000
otel-collector_1  | ExplicitBounds #10: 1000.000000
otel-collector_1  | ExplicitBounds #11: 2500.000000
otel-collector_1  | ExplicitBounds #12: 5000.000000
otel-collector_1  | ExplicitBounds #13: 7500.000000
otel-collector_1  | ExplicitBounds #14: 10000.000000

After (with the fix):

otel-collector_1  | ExplicitBounds #0: 0.050000
otel-collector_1  | ExplicitBounds #1: 0.100000
otel-collector_1  | ExplicitBounds #2: 0.500000
otel-collector_1  | ExplicitBounds #3: 1.000000
otel-collector_1  | ExplicitBounds #4: 1.500000
otel-collector_1  | ExplicitBounds #5: 2.000000
otel-collector_1  | ExplicitBounds #6: 2.500000
otel-collector_1  | ExplicitBounds #7: 3.000000
otel-collector_1  | ExplicitBounds #8: 4.000000
otel-collector_1  | ExplicitBounds #9: 5.000000
otel-collector_1  | ExplicitBounds #10: 10.000000
otel-collector_1  | ExplicitBounds #11: 20.000000
otel-collector_1  | ExplicitBounds #12: 30.000000
otel-collector_1  | ExplicitBounds #13: 40.000000
otel-collector_1  | ExplicitBounds #14: 50.000000
otel-collector_1  | ExplicitBounds #15: 60.000000
otel-collector_1  | ExplicitBounds #16: 300.000000
otel-collector_1  | ExplicitBounds #17: 600.000000

Does This PR Require a Contrib Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@Dudssource Dudssource requested a review from a team as a code owner December 27, 2023 05:43
Copy link

linux-foundation-easycla bot commented Dec 27, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@srikanthccv srikanthccv changed the title Fix #3522 OTLPMetricExporter ignores preferred_aggregation property Fix OTLPMetricExporter ignores preferred_aggregation property Dec 31, 2023
@srikanthccv
Copy link
Member

srikanthccv commented Dec 31, 2023

Let's add a unit test and CHANGELOG entry

Dudssource added a commit to Dudssource/opentelemetry-python that referenced this pull request Jan 8, 2024
@Dudssource
Copy link
Contributor Author

Let's add a unit test and CHANGELOG entry

Thanks @srikanthccv for your feedback!
I just added a changelog entry and a unit test, pls let me know if I need to add something else.

srikanthccv
srikanthccv previously approved these changes Jan 22, 2024
@srikanthccv srikanthccv dismissed their stale review January 27, 2024 17:14

new changes after review

@Dudssource
Copy link
Contributor Author

Dudssource commented Jan 27, 2024

Linter is now passing (0195013)

@Dudssource
Copy link
Contributor Author

Pylint linter fixed on d60819a

@ocelotl ocelotl force-pushed the fix-otlpexporter-preferred-aggregation branch from 967521a to af1de86 Compare February 13, 2024 23:32
ocelotl pushed a commit to Dudssource/opentelemetry-python that referenced this pull request Feb 13, 2024
@ocelotl ocelotl force-pushed the fix-otlpexporter-preferred-aggregation branch from ba8cd42 to 479e28e Compare February 13, 2024 23:43
@ocelotl ocelotl enabled auto-merge (squash) February 13, 2024 23:56
auto-merge was automatically disabled February 14, 2024 00:00

Pull Request is not mergeable

@ocelotl ocelotl merged commit 12f4490 into open-telemetry:main Feb 14, 2024
270 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OTLPMetricExporter ignores preferred_aggregation
5 participants