feat(sdk-metrics): implement MetricProducer specification #4007

aabmass · 2023-07-19T15:54:43Z

Which problem is this PR solving?

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #4006

Short description of the changes

Allows adding additional MetricProducers to a MetricReader which can be used to produce additional data beyond what the SDK produces

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Added new tests cases

Checklist:

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

aabmass · 2023-07-19T15:58:45Z

packages/sdk-metrics/src/export/MetricReader.ts

+   * Additional MetricProducers to use as a source of aggregated metric data in addition to the
+   * SDK's metric data.
+   */
+  metricProducers?: MetricProducer[];


Note the specification asks for a RegisterMetricProducer function https://github.com/open-telemetry/opentelemetry-specification/blob/v1.23.0/specification/metrics/sdk.md#registerproducermetricproducer

However, I since we already have a public setMetricProducer which assumes it is receiving the SDK's MetricCollector, this seems like the cleanest solution without breaking anything.

I will also check with the spec on this.

Opened open-telemetry/opentelemetry-specification#3611

I also think having it as a config makes the most sense in our case. If the spec wording gets changed then I'd much prefer having it be config-only. 🙂

open-telemetry/opentelemetry-specification#3613 has a few approvals now

codecov · 2023-07-19T16:00:58Z

Codecov Report

Merging #4007 (694b411) into main (48fb158) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 694b411 differs from pull request most recent head f095627. Consider uploading reports for the commit f095627 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4007   +/-   ##
=======================================
  Coverage   92.36%   92.36%           
=======================================
  Files         321      320    -1     
  Lines        9264     9251   -13     
  Branches     1968     1963    -5     
=======================================
- Hits         8557     8545   -12     
+ Misses        707      706    -1

Files Changed	Coverage Δ
packages/sdk-metrics/src/export/MetricReader.ts	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

legendecas · 2023-07-21T14:48:28Z

packages/sdk-metrics/src/export/MetricReader.ts

+        ),
+      ]);
+
+    // Merge the results, keeping the SDK's Resource


I think we should add multiple resources support to the metric reader and metric exporter instead of dropping resources silently -- the registered metric producer can be different meter providers that have different resources.

I'm not sure how we could do that without making a breaking change. Also see this part of the spec:

If the MeterProvider is an instance of MetricProducer, this MAY be used to register the MeterProvider, but MUST NOT allow multiple MeterProviders to be registered with the same MetricReader.

That's not exactly the case in the JS implementation, but I think the spirit of it is to avoid this happening. I was actually thinking of making the MetricProducer only able to return ScopeMetrics to avoid the issue altogether.

Fyi open-telemetry/opentelemetry-specification#3636

I was actually thinking of making the MetricProducer only able to return ScopeMetrics to avoid the issue altogether.

I like this idea. However, I wonder if we could do this without breaking the MeterProvider as it also implements the MetricProducer interface. 🤔

It's a tricky situation, but I think we can leave this as-is and then iterate later depending on the outcome of open-telemetry/opentelemetry-specification#3636 - the code proposed here never alters anything unless the user opts into using an experimental feature by setting the metricProducers config - I think in that case it would be okay to change the behavior later.

Is this OK with you for now @legendecas?

packages/sdk-metrics/src/export/MetricReader.ts

pichlermarc

Thank you for working on this. 🙂

A few questions came up while I was reviewing:

is it always expected that people use MetricProducer with an SDK (MeterProvider), or also without one? 🤔

In that case, the PeriodicExportingMetricReader would only start once setMetricProducer() is called manually, right? Or would MetricProducers be expected to register themselves with the reader as the MeterProvider does in some SDK implementations (ours uses setMetricProducer(), I think in Python the MeterProvider registers a callback with the MetricReaders)?

Would it make sense to have similar ergonomics to a MeterProvider to handle this registration? 🤔

pichlermarc · 2023-08-02T14:53:18Z

packages/sdk-metrics/src/export/MetricReader.ts

+        ),
+      ]);
+
+    // Merge the results, keeping the SDK's Resource


I was actually thinking of making the MetricProducer only able to return ScopeMetrics to avoid the issue altogether.

I like this idea. However, I wonder if we could do this without breaking the MeterProvider as it also implements the MetricProducer interface. 🤔

It's a tricky situation, but I think we can leave this as-is and then iterate later depending on the outcome of open-telemetry/opentelemetry-specification#3636 - the code proposed here never alters anything unless the user opts into using an experimental feature by setting the metricProducers config - I think in that case it would be okay to change the behavior later.

packages/sdk-metrics/src/export/MetricReader.ts

pichlermarc · 2023-08-03T09:38:05Z

packages/sdk-metrics/src/export/MetricReader.ts

+   * Additional MetricProducers to use as a source of aggregated metric data in addition to the
+   * SDK's metric data.
+   */
+  metricProducers?: MetricProducer[];


I also think having it as a config makes the most sense in our case. If the spec wording gets changed then I'd much prefer having it be config-only. 🙂

legendecas

Thank you for working on this!

packages/sdk-metrics/src/export/MetricReader.ts

Co-authored-by: David Ashpole <dashpole@google.com>

aabmass · 2023-08-08T16:10:23Z

Thanks for the review everyone. I noticed you left a few questions I forgot to answer @pichlermarc

Or would MetricProducers be expected to register themselves with the reader as the MeterProvider does in some SDK implementations (ours uses setMetricProducer(), I think in Python the MeterProvider registers a callback with the MetricReaders)?

I don't think we should allow this, IMO passing the MetricProducers when creating the MeterProvider is cleaner for the callers. Actually I think I should add a doc comment to setMetricProducer() like "this should only be called internally by the SDK". Wdyt?

packages/sdk-metrics/src/index.ts

pichlermarc · 2023-08-09T14:03:07Z

Thanks for the review everyone. I noticed you left a few questions I forgot to answer @pichlermarc

Or would MetricProducers be expected to register themselves with the reader as the MeterProvider does in some SDK implementations (ours uses setMetricProducer(), I think in Python the MeterProvider registers a callback with the MetricReaders)?

I don't think we should allow this, IMO passing the MetricProducers when creating the MeterProvider is cleaner for the callers.

Hmm, the reason I was asking is mostly regarding a possible stand-alone use of a MetricProducer: is this something that is intended? If we decide against using the setMetricProducer() we most likely need some other mechanism to start the PeriodicExportingMetricReader. Right now on the PeriodicExportingMetricReader onInitialized() is only called when the MeterProvider is set via setMetricProducer(). 🤔

Actually I think I should add a doc comment to setMetricProducer() like "this should only be called internally by the SDK". Wdyt?

I think this is a good idea. 👍 I think there's also a jsdoc annotation @internal (or similar) that could be used for that. 🤔

aabmass · 2023-08-09T16:30:58Z

I think this is a good idea. 👍 I think there's also a jsdoc annotation @internal (or similar) that could be used for that. 🤔

Pushed f095627 with this change.

pichlermarc · 2023-08-10T08:24:45Z

Hmm, the reason I was asking is mostly regarding a possible stand-alone use of a MetricProducer: is this something that is intended? If we decide against using the setMetricProducer() we most likely need some other mechanism to start the PeriodicExportingMetricReader. Right now on the PeriodicExportingMetricReader onInitialized() is only called when the MeterProvider is set via setMetricProducer(). 🤔

Summary from the SIG: It's like this on purpose to discourage circumventing the SDK. Users shouldn't use the MetricProducer without a MeterProvider, therefore changes to the PeriodicExportingMetricReader are not necessary. 🙂

aabmass force-pushed the oc-shim-metrics1 branch from 0271b33 to b0434be Compare July 19, 2023 15:56

aabmass commented Jul 19, 2023

View reviewed changes

aabmass mentioned this pull request Jul 19, 2023

Clarify if MetricProducers can be registered with config instead of a method open-telemetry/opentelemetry-specification#3611

Closed

aabmass force-pushed the oc-shim-metrics1 branch 2 times, most recently from 81a7c59 to 49ac289 Compare July 19, 2023 22:31

feat(sdk-metrics): implement MetricProducer specification

2a5a31c

aabmass force-pushed the oc-shim-metrics1 branch from 49ac289 to 2a5a31c Compare July 19, 2023 22:50

aabmass marked this pull request as ready for review July 20, 2023 00:31

aabmass requested a review from a team as a code owner July 20, 2023 00:31

legendecas reviewed Jul 21, 2023

View reviewed changes

dashpole approved these changes Aug 1, 2023

View reviewed changes

packages/sdk-metrics/src/export/MetricReader.ts Outdated Show resolved Hide resolved

pichlermarc reviewed Aug 3, 2023

View reviewed changes

legendecas reviewed Aug 3, 2023

View reviewed changes

packages/sdk-metrics/src/export/MetricReader.ts Outdated Show resolved Hide resolved

aabmass and others added 3 commits August 8, 2023 11:19

Update packages/sdk-metrics/src/export/MetricReader.ts

16381c1

Co-authored-by: David Ashpole <dashpole@google.com>

Merge branch 'main' into oc-shim-metrics1

8af0460

Address review comments

4ee2de1

aabmass requested review from pichlermarc and legendecas August 8, 2023 15:33

legendecas approved these changes Aug 8, 2023

View reviewed changes

aabmass commented Aug 8, 2023

View reviewed changes

packages/sdk-metrics/src/index.ts Outdated Show resolved Hide resolved

Update packages/sdk-metrics/src/index.ts

15c1d39

Update setMetricProducer doc string to mark it internal

f095627

pichlermarc approved these changes Aug 9, 2023

View reviewed changes

pichlermarc merged commit d3436bf into open-telemetry:main Aug 10, 2023
15 checks passed

aabmass deleted the oc-shim-metrics1 branch August 10, 2023 19:15

dashpole mentioned this pull request Aug 10, 2023

Promote MetricProducer specification to feature-freeze open-telemetry/opentelemetry-specification#3655

Merged

aabmass mentioned this pull request Aug 23, 2023

feat: update PeriodicExportingMetricReader and PrometheusExporter to accept optional metric producers #4077

Merged

4 tasks

MrAlias mentioned this pull request Aug 31, 2023

Stabilize the MetricProducer open-telemetry/opentelemetry-specification#3599

Closed

pichlermarc mentioned this pull request Sep 6, 2023

chore: prepare release API 1.5.0, SDK 1.16.0/0.42.0 #4122

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sdk-metrics): implement MetricProducer specification #4007

feat(sdk-metrics): implement MetricProducer specification #4007

feat(sdk-metrics): implement MetricProducer specification #4007

feat(sdk-metrics): implement MetricProducer specification #4007

Conversation

Which problem is this PR solving?

Short description of the changes

Type of change

How Has This Been Tested?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment