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

BatchObserver semantics and usage patterns need clarification #1090

Closed
jkwatson opened this issue Oct 13, 2020 · 7 comments
Closed

BatchObserver semantics and usage patterns need clarification #1090

jkwatson opened this issue Oct 13, 2020 · 7 comments
Assignees
Labels
area:api Cross language API specification issue priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:metrics Related to the specification/metrics directory

Comments

@jkwatson
Copy link
Contributor

What are you trying to achieve?

The specification for the required behavior of BatchObserver and associated instruments is currently underspecified. See this PR attempt at clarification for additional context: #1030 .

In particular, we need to make it clear that Observer instruments that are created via a BatchObserver can only have callbacks that are handled by that BatchObserver, and no other callback can be associated with them. This may end up changing the semantics of the BatchObserver itself, and the patterns for instrumentation creation that it exposes.

@jkwatson jkwatson added the spec:metrics Related to the specification/metrics directory label Oct 13, 2020
@arminru arminru added the area:api Cross language API specification issue label Oct 14, 2020
@andrewhsu andrewhsu added priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Oct 16, 2020
@andrewhsu andrewhsu added this to Required for GA, needs action. Add label:release:required-for-ga to issues and PRs when moving them to this column. in GA Spec Burndown Oct 16, 2020
@jkwatson
Copy link
Contributor Author

@thisthat would you be willing to collaborate on writing the spec update for this, since you've been working on the Java implementation?

@thisthat
Copy link
Member

@jkwatson I am down to collaborate on the spec. However, I don't feel confident driving it because I just start to look into the metric part of OTel.

@jkwatson
Copy link
Contributor Author

@jkwatson I am down to collaborate on the spec. However, I don't feel confident driving it because I just start to look into the metric part of OTel.

ok, I'll try to write something up, and I'll tag you on the PR

@kenfinnigan
Copy link
Member

@jkwatson I've been spending some time looking at this and the related issues/PRs today and I had a couple of comments.

Batch Observer in the spec is confusing from looking at the example as it doesn't appear it would even "compile" as it's referencing the observers inside the callback before they're even created.

It makes me wonder whether the Batch observer shouldn't have the ability to create observers and only associate them, as to me it appears to be a "chicken and egg" type problem.

I think I saw it mentioned by @jmacd in another issue, that it would be good to have a .start() on the Batch observer after which point no more meter associations can be made.

Happy to help with spec clarification, but wanting to make sure I'm understanding the problem first

@jkwatson
Copy link
Contributor Author

I think the goal is to make sure that only one callback is registered per instrument. I think this is also an issue with the standard Observers, honestly.

One approach, I think, is to not leak the instruments out of the creation flow at all, so that no references to instruments are exposed once they have been created and a callback has been registered. I don't know if there are use-cases that would be extra difficult to implement if we did that, though.

@jkwatson
Copy link
Contributor Author

jkwatson commented Mar 5, 2021

I'm going to un-assign this from me, since I am not spending any time on it.

@reyang
Copy link
Member

reyang commented Nov 7, 2021

As we're releasing the Metrics API as stable #2104 (which doesn't have BatchObserver), closing this issue.

@reyang reyang closed this as completed Nov 7, 2021
GA Spec Burndown automation moved this from Required/Allowed for GA, todo. to Required/Allowed for GA, resolved. Nov 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:metrics Related to the specification/metrics directory
Projects
No open projects
GA Spec Burndown
  
Required/Allowed for GA, resolved.
Development

No branches or pull requests

6 participants