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

Histogram Aggregation on Up-Down-Counters results in wrong max #3085

Closed
pichlermarc opened this issue Jul 11, 2022 · 1 comment · Fixed by #3086
Closed

Histogram Aggregation on Up-Down-Counters results in wrong max #3085

pichlermarc opened this issue Jul 11, 2022 · 1 comment · Fixed by #3086
Assignees
Labels
bug Something isn't working priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect
Milestone

Comments

@pichlermarc
Copy link
Member

What happened?

HistogramAggregation incorrectly assumes positive-only values, and therfore produces wrong max values on export.

Steps to Reproduce

  1. Set up OTel JS Metrics
  2. Create View that changes an UpDownCounter to have an ExplicitBucketHistogramAggregation
  3. Create an UpDownCounter
  4. Only add values -1

Expected Result

Exported metrics should have a maximum of < -1.

Actual Result

Exported metrics have a maximum of -1.

OpenTelemetry Setup Code

// Test for 'MeterProvider addView' in 'MeterProvider.test.ts'

it('with UP_DOWN_COUNTER and ExplicitBucketHistogramAggregation does export the correct min/max', async() => {
      const meterProvider = new MeterProvider({ resource: defaultResource });
      const reader = new TestMetricReader();
      meterProvider.addMetricReader(reader);

      meterProvider.addView({
        aggregation: new ExplicitBucketHistogramAggregation([1,3,4])
      },
      {
        instrument: {
          name: 'test-histogram'
        },
        meter: {
          name: 'meter1'
        }
      });

      // Create meter and instruments.
      const meter = meterProvider.getMeter('meter1', 'v1.0.0');
      const counter = meter.createUpDownCounter('test-histogram');

      // Record value.
      counter.add(-20);
      counter.add(-5);

      // Perform collection.
      const { resourceMetrics, errors } = await reader.collect();

      assert.strictEqual(errors.length, 0);
      // Results came only from one Meter.
      assert.strictEqual(resourceMetrics.scopeMetrics.length, 1);

      // InstrumentationScope matches the only created Meter.
      assertScopeMetrics(resourceMetrics.scopeMetrics[0], {
        name: 'meter1',
        version: 'v1.0.0'
      });

      // one metric collected
      assert.strictEqual(resourceMetrics.scopeMetrics[0].metrics.length, 1);

      const metric = resourceMetrics.scopeMetrics[0].metrics[0];
      assert.strictEqual(metric.dataPointType, DataPointType.HISTOGRAM);
      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
      // @ts-ignore was asserted to be DataPointType.HISTOGRAM
      assert.strictEqual(metric.dataPoints[0].value.min, -20);
      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
      // @ts-ignore was asserted to be DataPointType.HISTOGRAM
      assert.strictEqual(metric.dataPoints[0].value.max, -5);
    });

// test for Histogram.test.ts, 'HistogramAggregator merge'
it('with only negatives', () => {
      const aggregator = new HistogramAggregator([1, 10, 100], true);
      const prev = aggregator.createAccumulation([0, 0]);
      prev.record(-10);
      prev.record(-20);

      const delta = aggregator.createAccumulation([1, 1]);
      delta.record(-5);
      delta.record(-30);

      assert.deepStrictEqual(aggregator.merge(prev, delta).toPointValue(), {
        buckets: {
          boundaries: [1, 10, 100],
          counts: [4, 0, 0, 0]
        },
        count: 4,
        hasMinMax: true,
        max: -5,
        min: -30,
        sum: -65
      });
    });

package.json

"@opentelemetry/sdk-metrics-base": "0.30.0"

Relevant log output

No response

@pichlermarc
Copy link
Member Author

Provided a fix in #3086.

@dyladan dyladan added priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect and removed triage labels Jul 11, 2022
@dyladan dyladan added this to the Metrics GA milestone Jul 11, 2022
@pichlermarc pichlermarc self-assigned this Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect
Projects
None yet
2 participants