feat(sdk-metrics): adds the cardinalitySelector argument to PeriodicExportingMetricReaders

[starzlocker]

Which problem is this PR solving?

This PR adds support in the PeriodicExportingMetricReaders for the cardinalitySelector argument, which is passed to the MetricReader class. This allows this value to be specified in the PeriodicExportingMetricReaders.

Fixes #6425

Short description of the changes

Type of change

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

I've introduced 2 unit tests, one to assert that when cardinalitySelector is not specified, the fallback value of the MetricReader.selectCardinalitySelector is respected. I've also tested to assure that when a value is provided, it has preference over the default.

Checklist:

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

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: maryliag / name: Marylia Gutierrez (9ca22be, f5c11e3)
• ✅ login: starzlocker / name: Adolfo Gomes (712f263, 9ca22be)
• ✅ login: starzlocker / name: adolfogomes (03a4bba, 38a120e, 621f6f6, 62b20c2, 6b9f8d4, 6de4deb, 75696cb, 98d2c46, b934c8b, c7c9cca, c8dde4e, cc10bfa, cf40adb, eb62521, ef66406, f2f3562, fb067a4)

[trentm]

Thanks! LGTM, though I'm not (yet) "approving": I'd like @pichlermarc's sanity check on Metrics SDK things (at least while I'm still a wimp on Metrics).

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.75%. Comparing base (84bfa90) to head (f5c11e3).
⚠️ Report is 40 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #6460    +/-   ##
========================================
  Coverage   95.75%   95.75%            
========================================
  Files         364      375    +11     
  Lines       12095    12725   +630     
  Branches     2884     3013   +129     
========================================
+ Hits        11581    12185   +604     
- Misses        514      540    +26     

Files with missing lines	Coverage Δ
...etrics/src/export/PeriodicExportingMetricReader.ts	98.73% <100.00%> (+0.45%)	⬆️

... and 32 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[maryliag]

Thank you for picking up the issue.
I wouldn't say this "fixes" the issue (which is about cardinality limits, not cardinality selector), meaning you're passing the value for the cardinality selector, not the limits. To give an example, during the config you can set the limits like this:

cardinality_limits: {
            default: 2000,
            counter: 2000,
            gauge: 2000,
            histogram: 2000,
            observable_counter: 2000,
            observable_gauge: 2000,
            observable_up_down_counter: 2000,
            up_down_counter: 2000,
          },

so it still need some way to pass the value of the limit

[pichlermarc]

@maryliag, yep looks like the current proposal would work like this:

 new PeriodicExportingMetricReader({
    exporter,
    exportIntervalMillis: 1,
    exportTimeoutMillis: 1,
    cardinalitySelector: (instrumentType: InstrumentType) => {
      switch (instrumentType) {
        case InstrumentType.COUNTER:
          return 5000;
        case InstrumentType.GAUGE:
          return 2000;
        case InstrumentType.HISTOGRAM:
          return 100;
        default:
          return 1000;
      }
    },
  });

I'm personally not really a fan of the function-based selectors as they need to be pure, but it's not enforced; it can be confusing to users. We might as well just pass options. Though that adds another type to the public API.

Something like:

 new PeriodicExportingMetricReader({
    exporter,
    exportIntervalMillis: 1,
    exportTimeoutMillis: 1,
    cardinalityLimits: {
        counter: 5000,
        gauge: 2000,
        histogram: 100,
        default: 1000
    },
  });

seems much cleaner and makes it clear that re-configuring on runtime is not supported. plus it's likely easier to use on the declarative config side.

suggestion: let's pass options instead, then wrap these in a selector to comply with the current IMetricReader interface.

[starzlocker]

hi, everyone, after reading your suggestions, i allowed cardinalityLimits to be passed to the constructor and then i use it's values to build the cardinalitySelector that is passed on to the MetricReader

cardinalitySelector: (instrumentType: InstrumentType) => {
  const limits = cardinalityLimits ?? {
    counter: 2000,
    gauge: 2000,
    histogram: 2000,
    default: 2000,
  };

  switch (instrumentType) {
    case InstrumentType.COUNTER:
      return limits.counter;
    case InstrumentType.GAUGE:
      return limits.gauge;
    case InstrumentType.HISTOGRAM:
      return limits.histogram;
    default:
      return limits.default;
  }
},

[MikeGoldsmith]

Looks pretty good - left some suggestions.

[starzlocker]

• Includes the missing InstrumentTypes and made them optional

  cardinalityLimits?: {
    counter?: number;
    gauge?: number;
    histogram?: number;
    upDownCounter?: number;
    observableCounter?: number;
    observableGauge?: number;
    observableUpDownCounter?: number;
    default?: number;
  };

• Refactors the cardinalitySelector wrapper accordingly:

      cardinalitySelector: (instrumentType: InstrumentType) => {
        const limits = {
          counter: 2000,
          gauge: 2000,
          histogram: 2000,
          upDownCounter: 2000,
          observableCounter: 2000,
          observableGauge: 2000,
          observableUpDownCounter: 2000,
          default: 2000,
          ...(cardinalityLimits ?? {}),
        };

        switch (instrumentType) {
          case InstrumentType.COUNTER:
            return limits.counter;
          case InstrumentType.GAUGE:
            return limits.gauge;
          case InstrumentType.HISTOGRAM:
            return limits.histogram;
          case InstrumentType.OBSERVABLE_COUNTER:
            return limits.observableCounter;
          case InstrumentType.OBSERVABLE_UP_DOWN_COUNTER:
            return limits.observableUpDownCounter;
          case InstrumentType.OBSERVABLE_GAUGE:
            return limits.observableGauge;
          case InstrumentType.UP_DOWN_COUNTER:
            return limits.upDownCounter;
          default:
            return limits.default;
        }
      },
    });

• Renamed the test case with 'should use provided cardinalityLimits over defaults';

• Altered the import statement to:

import type { MetricProducer } from './MetricProducer';

[MikeGoldsmith]

🚀

[maryliag]

a few nits

[maryliag]

just missing changelog and should be good to go 😄

[maryliag]

oh can you also update readme to show example of usage? that would be great!

[starzlocker]

i altered the readme, i don't know if it's too much or too little haha

please, let me know

[maryliag]

Awesome! Thank for a great first contribution! 😄

[starzlocker]

Just checking in on this PR.

It was approved a few days ago, and I wanted to confirm if there’s anything else needed from my side before it can be merged.

If there are any concerns or additional changes required, I’m happy to address them

[otelbot]

Thank you for your contribution @starzlocker! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey.