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

Add basic integration test for OtlpMeterRegistry and the OTel collector #3668

Merged
merged 4 commits into from
Feb 24, 2023

Conversation

jonatan-ivanov
Copy link
Member

@jonatan-ivanov jonatan-ivanov commented Feb 24, 2023

Resolves #3534

@jonatan-ivanov jonatan-ivanov added enhancement A general enhancement registry: otlp OpenTelemetry Protocol (OTLP) registry-related labels Feb 24, 2023
@jonatan-ivanov jonatan-ivanov added this to the 1.9.9 milestone Feb 24, 2023
matchesPattern("(?s)^.*test_gauge\\{.+} 12\\n.*$"),

matchesPattern("(?s)^.*test_timer_count\\{.+} 1\\n.*$"),
matchesPattern("(?s)^.*test_timer_sum\\{.+} 123\\n.*$"),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I'm looking at this, I don't think this is correct, this should be 123ms not 123s.

Copy link
Member Author

@jonatan-ivanov jonatan-ivanov Feb 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a collector issue, the data that we send looks like this:

metrics {
  name: "test.timer"
  unit: "milliseconds"
  histogram {
    data_points {
      start_time_unix_nano: 1677210838494000000
      time_unix_nano: 1677210839021000000
      count: 1
      sum: 123.0
    }
    aggregation_temporality: AGGREGATION_TEMPORALITY_CUMULATIVE
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I guess we'll need a fix in the collector before we can change the test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like it might be intentional that no conversion is done currently. But then there's no way to know the unit as far as I can tell.
https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/pkg/translator/prometheus

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if there are receivers that mandate a unit that is not seconds.
Or two receivers that mandate different units. Or two exporters with different units.
In all of these cases I think not doing the conversion can lead to wrong result.

@shakuzen shakuzen linked an issue Feb 24, 2023 that may be closed by this pull request
@shakuzen shakuzen removed this from the 1.9.9 milestone Feb 24, 2023
@shakuzen shakuzen removed the enhancement A general enhancement label Feb 24, 2023
// @formatter:off
return given()
.port(container.getMappedPort(9090))
.accept(OPENMETRICS_100)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I tried running the collector locally, I was not able to get it to return OpenMetrics format even when sending this Accept header and configuring the prometheus exporter with enable_open_metrics: true. Debugging this test, it looks like the response is also not OpenMetrics format.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch, I did not even notice this. This is another bug in the collector.
If I send 1.0.0 (what I should), it does not work:

Accept: application/openmetrics-text; version=1.0.0; charset=utf-8

If I send 0.0.1 (what I should not), it works:

Accept: application/openmetrics-text; version=0.0.1; charset=utf-8

See details here: open-telemetry/opentelemetry-collector-contrib#18913

Copy link
Member Author

@jonatan-ivanov jonatan-ivanov Feb 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this by requesting (and verifying) 0.0.1 and also included a TODO item.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
registry: otlp OpenTelemetry Protocol (OTLP) registry-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integration test for OTLP registry
2 participants