feature(views): move views registration to MeterProvider constructor #3066

pichlermarc · 2022-06-29T15:17:50Z

Which problem is this PR solving?

When create<instrument_name_here>() is called before addView(), the View is not applied to the instrument.
Having addView() on the MeterProvider the gives the wrong impression that this can be done after creating instruments.

This PR moves the functionality provided by addView() to the MeterProvider constructor. This is how most SDKs (Python, Java, .NET) handle this today.

This makes it clear that views have to be created before creating instruments (as no Meter, and therefore no Instrument can be created without a MeterProvider), and brings us in line with other SDKs.

Also fixes a specification inconsistency where views would allow empty instrument/meter selectors, but not empty view options (see spec here).

Fixes #3056

Short description of the changes

Moves View registration to the constructor
Moves checking for multiple instrument selection on named views to the View class.
Combines the ViewOptions and SelectorOptions into one interface to reduce nesting when creating Views
Allows empty View configuration (all empty name, aggregation, description, attributeKeys)
Disallows empty Instrument selectors (all empty instrumentName, instrumentType, meterName, meterSchemaUrl, meterVersion)

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Existing tests

Checklist:

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

codecov · 2022-06-29T15:43:58Z

Codecov Report

Merging #3066 (da511f3) into main (db32860) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3066      +/-   ##
==========================================
+ Coverage   93.08%   93.09%   +0.01%     
==========================================
  Files         188      188              
  Lines        6261     6256       -5     
  Branches     1323     1319       -4     
==========================================
- Hits         5828     5824       -4     
+ Misses        433      432       -1

Impacted Files	Coverage Δ
...pentelemetry-sdk-metrics-base/src/MeterProvider.ts	`100.00% <100.00%> (ø)`
...es/opentelemetry-sdk-metrics-base/src/view/View.ts	`100.00% <100.00%> (ø)`
...elemetry-sdk-metrics-base/src/view/ViewRegistry.ts	`100.00% <100.00%> (ø)`
...emetry-core/src/platform/node/RandomIdGenerator.ts	`93.75% <0.00%> (+6.25%)`	⬆️

experimental/packages/opentelemetry-sdk-metrics-base/src/MeterProvider.ts

legendecas · 2022-07-08T07:20:21Z

experimental/packages/opentelemetry-sdk-metrics-base/src/view/View.ts

+export type ViewOptions = {
+  // View
+  /**
+   *  If not provided, the Instrument name will be used by default. This will be used as the name of the metrics stream.


Note, after this change, the options object does contain an instrumentName that can be confused with the term "Instrument name" in this statement.

Suggested change

* If not provided, the Instrument name will be used by default. This will be used as the name of the metrics stream.

* If not provided, the original Instrument name will be used by default. This will be used as the name of the metrics stream.

Or can we update the instrumentName to be selectInstrumentName or something else to clarify that?

Thanks for pointing this out. I updated the documentation in 59e69ae. 🙂
I hope that the documentation is sufficient, but I'm not opposed to changing the name too.

experimental/packages/opentelemetry-sdk-metrics-base/src/view/View.ts

legendecas · 2022-07-08T07:29:25Z

experimental/packages/opentelemetry-sdk-metrics-base/src/view/View.ts

+  constructor(viewOptions: ViewOptions) {
+    // If no criteria is provided, the SDK SHOULD treat it as an error.
+    // It is recommended that the SDK implementations fail fast.
+    if (isSelectorProvided(viewOptions)) {


I'm confused by this assertion. Views without name specified should be allowed to not set instrument selection criteria. Like:

new View({ aggregation: DEFAULT_AGGREGATION });

I fully agree, I was quite surprised as well when I re-read the spec and found that this should not be allowed.

Here it states, in context of instrument selection criteria that:

If no criteria is provided, the SDK SHOULD treat it as an error. It is
recommended that the SDK implementations fail fast. Please refer to Error
handling in OpenTelemetry for the general guidance.

That being said: it is not completely impossible to get the same outcome as before. It now requires providing a wildcard instrumentName like so:

new View({ aggregation: DEFAULT_AGGREGATION, instrumentName: '*' });

So even with this change to make it comply with the spec, the functionality is still retained, but I agree that this specific case is more confusing now than it was before. 😕

experimental/packages/opentelemetry-sdk-metrics-base/src/view/View.ts

legendecas · 2022-07-08T07:31:43Z

experimental/packages/opentelemetry-sdk-metrics-base/src/view/View.ts

-    this.description = config?.description;
-    this.aggregation = config?.aggregation ?? Aggregation.Default();
-    this.attributesProcessor = config?.attributesProcessor ?? AttributesProcessor.Noop();
+  constructor(viewOptions: ViewOptions) {


I find that isViewOptionsEmpty is no longer applied on creating the views. Is there any specific reason for removing that?

When I implemented isViewOptionsEmpty() in #2820 I read the spec wrong, and it is actually not disallowed to create an empty view, but it is disallowed to create view without selector options specified. I explain it in more detail and link to the spec section in question in this comment here.

legendecas · 2022-07-08T07:38:23Z

Allows empty View configuration (all empty name, aggregation, description, attributeKeys)

Disallows empty Instrument selectors (all empty instrumentName, instrumentType, meterName, meterSchemaUrl, meterVersion)

I'm confused by this change. IMO, empty view configuration is not meaningful -- it's just the default view and doesn't need to be added anyways, whilst empty view selection criteria are helpful in adding a custom default view.

Would you mind elaborating on this change?

pichlermarc · 2022-07-08T13:29:00Z

Allows empty View configuration (all empty name, aggregation, description, attributeKeys)

Disallows empty Instrument selectors (all empty instrumentName, instrumentType, meterName, meterSchemaUrl, meterVersion)

I'm confused by this change. IMO, empty view configuration is not meaningful -- it's just the default view and doesn't need to be added anyways, whilst empty view selection criteria are helpful in adding a custom default view.

Would you mind elaborating on this change?

I can see that this is an odd change. It comes from a misunderstanding I had when implementing it in #2820. More details can be found in this comment here.

legendecas

Code-wise LGTM. Still not sure about allowing empty views. But this is a trivial behavior, I'm not going to block this from landing.

pichlermarc · 2022-07-12T13:01:13Z

Code-wise LGTM. Still not sure about allowing empty views. But this is a trivial behavior, I'm not going to block this from landing.

Thank you for your review! 🙂

If you like I can open a discussion issue and maybe we can add it back in. I'm actually of the same opinion as you are on this. When I first implemented this check, it was a natural thing to check if the view is empty. In this PR I allowed empty views again as the spec does not explicitly prohibit it. Other SDKs (relevant code for Java, Python and .NET) also allow empty views, so we would be going against the grain on this.

…ructor-views

dyladan · 2022-07-12T13:27:11Z

An empty view is never selected right? Shouldn't they just be dropped? Maybe i'm misunderstanding

pichlermarc · 2022-07-12T13:29:18Z

An empty view is never selected right? Shouldn't they just be dropped? Maybe i'm misunderstanding

Yes, it is effectively a no-op. The changes here have the effect that creating empty views does not throw anymore.

Edit: formatting.

legendecas · 2022-07-12T14:52:41Z

An empty view is never selected right? Shouldn't they just be dropped? Maybe i'm misunderstanding

We have two conditions here:

(1) empty view configuration,
(2) empty view selection criteria.

Before this change, for the above conditions, we have:

(1) not allowed, because it is basically the same as the default view, no need to add it manually,
(2) allowed, interpreted as a wildcard selection for convenience on configuring a new default view like new View({ aggregation: MyAggregation }).

With this change, the conditions are on the contrary:

(1) allowed,
(2) not allowed.

Does the previous behavior of (2) sound ambiguous on whether it means "select all" or "select nothing" to you? If so, maybe the change in this PR made (2) more precise.

dyladan

LGTM I think this is way less likely to be misconfigured.

Can you update the README also?

experimental/packages/opentelemetry-sdk-metrics-base/src/MeterProvider.ts

experimental/packages/opentelemetry-sdk-metrics-base/src/view/View.ts

pichlermarc · 2022-07-13T14:22:31Z

LGTM I think this is way less likely to be misconfigured.

Can you update the README also?

Added a simple example to the README.md in dd13da0. 🙂

legendecas

Still LGTM

vmarchaud · 2022-07-15T09:13:42Z

I wonder if that would not create another problem for end user that want to change aggregation for auto instrumentations where they need to do it before importing instrumentations (which looks like the same issue for module monkey patching). Now user will need to have to construct trace and metrics provider, add view on metrics and then import instrumentations ?

PS: @pichlermarc you will need to rebase your branch

…ructor-views

pichlermarc · 2022-07-15T10:44:07Z

I wonder if that would not create another problem for end user that want to change aggregation for auto instrumentations where they need to do it before importing instrumentations (which looks like the same issue for module monkey patching). Now user will need to have to construct trace and metrics provider, add view on metrics and then import instrumentations ?

PS: @pichlermarc you will need to rebase your branch

@vmarchaud yes, but since addView() did not work retroactively on already created instruments, I think this problem existed already. Calling addView() on the MeterProvider before creating any Instruments was the only surefire way to have all of the views applied, and that meant that it had to be done before loading any instrumentations. It could be that I'm misunderstanding something, though. 🤔

One of my motivations in this PR is that it allows us to add an addView() method that reconfigures existing instruments to the MeterProvider in the future without it being a breaking change on the MeterProvider.

I rebased the branch. 🙂
Not sure why the tests are failing, I'm investigating.

vmarchaud · 2022-07-15T10:52:36Z

think this problem existed already

Yeah i understood this too with your PR, i though initially that adding views would reconfigure instrument.

One of my motivations in this PR is that it allows us to add an addView() method that reconfigures existing instruments to the MeterProvider in the future without it being a breaking change on the MeterProvider.

Cool to know its the end goal ! I was already imagining end users complaining that this isn't friendly so thats why i brought the subject :)

pichlermarc · 2022-07-18T12:22:41Z

Previously failing test seems to be related to lerna. It can sometimes be reproduced on main as well, seems unrelated to this PR. See lerna/lerna#3201

vmarchaud · 2022-07-18T13:53:34Z

@dyladan since you had nits since last review i'll let you merge if you are still good

pichlermarc added 2 commits July 4, 2022 15:29

feature(views): move views registration to MeterProvider constructor

a310e37

feature(views): introduce UserView class.

a44657e

pichlermarc force-pushed the meterprovider-constructor-views branch from cca5c73 to a44657e Compare July 4, 2022 13:30

pichlermarc added 9 commits July 4, 2022 16:51

feature(views): add tests for UserView.

4834310

feature(views): remove View class.

b8535a3

feature(views): rename UserView -> View.

ebe4d53

feature(views): update tests.

6c511ab

feature(views): collapse options into one.

84c3e85

feature(views): verify that at least one selector option is used.

a937ffc

feature(views): undo accidental re-format.

b48efde

feature(views): cleanup.

7a91b9d

fix(changelog): add changelog entry.

389521e

pichlermarc marked this pull request as ready for review July 5, 2022 16:33

pichlermarc requested a review from a team as a code owner July 5, 2022 16:33

pichlermarc added 2 commits July 7, 2022 16:13

Merge branch 'main' into meterprovider-constructor-views

38122b7

fix(changelog): move changelog entry.

bf7caab

pichlermarc mentioned this pull request Jul 7, 2022

View is not applied when instrument is created before addView() is called #3056

Closed

legendecas reviewed Jul 8, 2022

View reviewed changes

pichlermarc added 2 commits July 8, 2022 14:45

fix(views): address review comments.

59e69ae

fix(views): rename isSelectorProvided -> isSelectorNotProvided

dc2b657

fix(views): amend documentation.

7892706

legendecas approved these changes Jul 12, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into meterprovider-const…

600a49c

…ructor-views

dyladan approved these changes Jul 12, 2022

View reviewed changes

pichlermarc added 2 commits July 13, 2022 16:12

fix(meterprovider): address review comments.

dd13da0

docs(sdk-metrics-base): remove empty lines.

87271e8

legendecas approved these changes Jul 14, 2022

View reviewed changes

vmarchaud approved these changes Jul 15, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into meterprovider-const…

da511f3

…ructor-views

dyladan merged commit bec6c3c into open-telemetry:main Jul 18, 2022

dyladan deleted the meterprovider-constructor-views branch July 18, 2022 13:55

dyladan mentioned this pull request Jul 25, 2022

chore: release proposal 1.5.0 / 0.31.0 #3112

Merged

pichlermarc mentioned this pull request Sep 4, 2023

[sdk-metrics] Metric Instruments are not available in MetricReaders added after creation #4112

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(views): move views registration to MeterProvider constructor #3066

feature(views): move views registration to MeterProvider constructor #3066

pichlermarc commented Jun 29, 2022 •

edited

codecov bot commented Jun 29, 2022 •

edited

legendecas Jul 8, 2022

pichlermarc Jul 8, 2022

legendecas Jul 8, 2022

pichlermarc Jul 8, 2022

legendecas Jul 8, 2022

pichlermarc Jul 8, 2022

legendecas commented Jul 8, 2022

pichlermarc commented Jul 8, 2022

legendecas left a comment •

edited

pichlermarc commented Jul 12, 2022 •

edited

dyladan commented Jul 12, 2022

pichlermarc commented Jul 12, 2022 •

edited

legendecas commented Jul 12, 2022 •

edited

dyladan left a comment

pichlermarc commented Jul 13, 2022

legendecas left a comment

vmarchaud commented Jul 15, 2022

pichlermarc commented Jul 15, 2022

vmarchaud commented Jul 15, 2022

pichlermarc commented Jul 18, 2022

vmarchaud commented Jul 18, 2022

	* If not provided, the Instrument name will be used by default. This will be used as the name of the metrics stream.
	* If not provided, the original Instrument name will be used by default. This will be used as the name of the metrics stream.

feature(views): move views registration to MeterProvider constructor #3066

feature(views): move views registration to MeterProvider constructor #3066

Conversation

pichlermarc commented Jun 29, 2022 • edited

Which problem is this PR solving?

Short description of the changes

Type of change

How Has This Been Tested?

Checklist:

codecov bot commented Jun 29, 2022 • edited

Codecov Report

legendecas Jul 8, 2022

Choose a reason for hiding this comment

pichlermarc Jul 8, 2022

Choose a reason for hiding this comment

legendecas Jul 8, 2022

Choose a reason for hiding this comment

pichlermarc Jul 8, 2022

Choose a reason for hiding this comment

legendecas Jul 8, 2022

Choose a reason for hiding this comment

pichlermarc Jul 8, 2022

Choose a reason for hiding this comment

legendecas commented Jul 8, 2022

pichlermarc commented Jul 8, 2022

legendecas left a comment • edited

Choose a reason for hiding this comment

pichlermarc commented Jul 12, 2022 • edited

dyladan commented Jul 12, 2022

pichlermarc commented Jul 12, 2022 • edited

legendecas commented Jul 12, 2022 • edited

dyladan left a comment

Choose a reason for hiding this comment

pichlermarc commented Jul 13, 2022

legendecas left a comment

Choose a reason for hiding this comment

vmarchaud commented Jul 15, 2022

pichlermarc commented Jul 15, 2022

vmarchaud commented Jul 15, 2022

pichlermarc commented Jul 18, 2022

vmarchaud commented Jul 18, 2022

pichlermarc commented Jun 29, 2022 •

edited

codecov bot commented Jun 29, 2022 •

edited

legendecas left a comment •

edited

pichlermarc commented Jul 12, 2022 •

edited

pichlermarc commented Jul 12, 2022 •

edited

legendecas commented Jul 12, 2022 •

edited