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

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

Closed
pichlermarc opened this issue Jun 23, 2022 · 4 comments · Fixed by #3066
Closed

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

pichlermarc opened this issue Jun 23, 2022 · 4 comments · Fixed by #3066
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@pichlermarc
Copy link
Member

Description

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.

Possible solution

Since view creation is considered part of the SDK initialization, we can move the functionality provided by addView() to the MeterProvider constructor. This is how most SDKs (Python, Java, .NET) handle this today.

This would make 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 would bring us in line with other SDKs.

@pichlermarc pichlermarc added the bug Something isn't working label Jun 23, 2022
@pichlermarc pichlermarc changed the title View is not applied when createInstrument() is called before addView() View is not applied when instrument is created before addView() is called Jun 23, 2022
@legendecas
Copy link
Member

legendecas commented Jun 28, 2022

The original thought of adding a view dynamically on a meter provider is that it can reduce the burden to set up a meter provider, like say similar to a metric reader or span processor -- we don't have to construct a metric reader or span processor strictly before the meter provider, which I think it is useful in practice.

However, as said specifically to views, maybe it is worth moving the view configuration to the constructor of the meter provider to reduce confusion about whether or not the view is applicable to previously created metric instruments.

Notably, I found that those SDKs in other languages also don't allow registering metric readers on the fly, while they are allowing span processors to be registered on the fly. AFAICT, the spec didn't put any constraints on this. If a span processor is registered after a span is created but before the span is ended, it can receive a span with onEnd without onStart beforehand.

I wondering if these problems are analogous and can be a problem with documentation.

@dyladan dyladan added enhancement New feature or request and removed bug Something isn't working labels Jun 28, 2022
@dyladan
Copy link
Member

dyladan commented Jun 28, 2022

This is not really a bug because it is working as designed, but is suggesting a change in design

@pichlermarc
Copy link
Member Author

@legendecas

Yes, I agree that this can also be a problem with documentation. While the spec does not put any constraints on when views can be added, I think moving the functionality of addView() to the constructor has some benefits:

  • it definitely prevents users from adding views "the wrong way".
  • we can decide to implement addView() to allow dynamic reconfiguration of views later. If we go GA with addView() not working on already created instruments, and we would want to add it alter, the resulting interface might be confusing as well. Updating addView() to allow dynamic reconfiguration would result in a breaking change, so we'd have to work around it with additional parameters, or a differnt addView() like method, that would probably cause even more confusion.

One other solution (other then solving it with documentation) would be to implement this dynamic reconfiguration of views now, but we'd have to do that before GA. However, I'm not sure if it is a good idea to even allow reconfiguration, as views have the power to alter the metric stream quite significantly.

For MetricReaders i think these problems do not apply, as, AFAIK, the MetricReader will still work when added afterwards, so there's no way for users to do it wrong there. 🙂

I have opened a PR (#3066) to show how it looks like in action if we would move it to the constructor. It does, as you said, increase the burden on creating a MeterProvider, but I think the tradeoff of preventing these errors is worth it. I'd be happy to get your opinion there as well. 🙂

@legendecas
Copy link
Member

@pichlermarc thank you for the explanation and the great work on this. I believe the proposed pattern can reduce the chance for people to get wrong with the views API. I'll take a look at the PR shortly.

@dyladan dyladan added this to the Metrics GA milestone Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants