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

Metrics instances can be created by direct instantiation #2550

Closed
ocelotl opened this issue Mar 24, 2022 · 7 comments · Fixed by #2844
Closed

Metrics instances can be created by direct instantiation #2550

ocelotl opened this issue Mar 24, 2022 · 7 comments · Fixed by #2844
Assignees
Labels
metrics sdk Affects the SDK package.

Comments

@ocelotl
Copy link
Contributor

ocelotl commented Mar 24, 2022

From the spec:

New Meter instances are always created through a MeterProvider

In our SDK, Meter (and other classes) can be instantiated directly, by importing the SDK Meter class and instantiating it, bypassing the MeterProvider. This is relevant to error handling, if we allow Meter to be instantiated directly, it may raise errors if the user passes bad parameters. If such thing happened using MeterProvider, the get_meter method could catch this errors and return a NoOpMeter.

@ocelotl ocelotl added sdk Affects the SDK package. metrics labels Mar 24, 2022
@srikanthccv
Copy link
Member

/s/meter/tracer/g - Same applies, we don't do anything to prevent this from happening.

@lzchen
Copy link
Contributor

lzchen commented Mar 25, 2022

We can do something similar as we did for spans? Also if we add this behavior for the tracer, would that be considered breaking?

@srikanthccv
Copy link
Member

Also if we add this behavior for the tracer, would that be considered breaking?

Yes, but I do not expect anyone to be directly instantiating it. IMO any changes that do not really fix any buggy behaviour but just move things around for the the sake of spec compliance is breaking. This is also why we can't address this issue #2463 without breaking user systems.

@srikanthccv srikanthccv moved this from Todo to In Progress in Python Metrics Stable Jul 17, 2022
@ocelotl
Copy link
Contributor Author

ocelotl commented Jul 18, 2022

Also if we add this behavior for the tracer, would that be considered breaking?

Yes, but I do not expect anyone to be directly instantiating it. IMO any changes that do not really fix any buggy behaviour but just move things around for the the sake of spec compliance is breaking. This is also why we can't address this issue #2463 without breaking user systems.

Are you in favor of closing this issue, then, @srikanthccv?

@srikanthccv
Copy link
Member

I was thinking may be we can ensure instruments are not created directly at least. What do you think?

@ocelotl
Copy link
Contributor Author

ocelotl commented Jul 19, 2022

Ok, I can work on that 👍

@ocelotl
Copy link
Contributor Author

ocelotl commented Jul 19, 2022

Sorry, I just noticed that you have assigned this to yourself. If you want/need I can take it, just let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
metrics sdk Affects the SDK package.
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants