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

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

Merged
merged 6 commits into from
Aug 10, 2023

Conversation

aabmass
Copy link
Member

@aabmass aabmass commented Jul 19, 2023

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

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

Choose a reason for hiding this comment

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

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.

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.

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. 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

@codecov
Copy link

codecov bot commented Jul 19, 2023

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

@aabmass aabmass marked this pull request as ready for review July 20, 2023 00:31
@aabmass aabmass requested a review from a team July 20, 2023 00:31
),
]);

// Merge the results, keeping the SDK's Resource
Copy link
Member

@legendecas legendecas Jul 21, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@aabmass aabmass Jul 22, 2023

Choose a reason for hiding this comment

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

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.

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.

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this OK with you for now @legendecas?

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

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

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? 🤔

),
]);

// Merge the results, keeping the SDK's Resource
Copy link
Member

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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. 🙂

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Thank you for working on this!

packages/sdk-metrics/src/export/MetricReader.ts Outdated Show resolved Hide resolved
@aabmass
Copy link
Member Author

aabmass commented Aug 8, 2023

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?

@pichlermarc
Copy link
Member

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
Copy link
Member Author

aabmass commented Aug 9, 2023

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
Copy link
Member

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. 🙂

@pichlermarc pichlermarc merged commit d3436bf into open-telemetry:main Aug 10, 2023
14 checks passed
@aabmass aabmass deleted the oc-shim-metrics1 branch August 10, 2023 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement MetricProducer specification
4 participants