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 instrument identity evaluation in MetricStorageRegistry#register #5683

Closed
jack-berg opened this issue Aug 4, 2023 · 0 comments · Fixed by #5701
Closed

Fix instrument identity evaluation in MetricStorageRegistry#register #5683

jack-berg opened this issue Aug 4, 2023 · 0 comments · Fixed by #5701

Comments

@jack-berg
Copy link
Member

As discussed in this comment, there's a bug in our evaluation of instrument identity in MetricStorageRegistry#register(..).

MetricStorageRegistry uses MetricDescriptor#equals(..) to determine if an existing metric storage should be used with a newly registered instrument. If yes, the new instrument's measurements will be merged with the existing. If no, a new storage is created and the instrument's measurements are associated with a new metric stream at collection.

I know of two things we're doing wrong:

  • If instruments are the same except for the case of a name, there should be a single metric stream exported, with the name of the first registered.
  • If instruments are the same except for advice, there should be a single metric stream exported, with the advice of the first registered.

The proof of the first issue is that the following test code fails:

    InMemoryMetricReader reader = InMemoryMetricReader.create();
    meterProvider = SdkMeterProvider.builder().registerMetricReader(reader).build();

    meterProvider.get("meter").counterBuilder("Counter").ofDoubles().build().add(1);
    meterProvider.get("meter").counterBuilder("counter").ofDoubles().build().add(1);

    assertThat(reader.collectAllMetrics())
        .satisfiesExactly(
            metric ->
                assertThat(metric)
                    .hasName("Counter")
                    .hasDoubleSumSatisfying(
                        sum ->
                            sum.hasPointsSatisfying(
                                point ->
                                    point.hasValue(2))));

Can produce similar test code to illustrate the second issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant