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

Nothing calls ObservableRegistry::Observe() #1550

Closed
ahadnagy opened this issue Aug 9, 2022 · 3 comments · Fixed by #1554
Closed

Nothing calls ObservableRegistry::Observe() #1550

ahadnagy opened this issue Aug 9, 2022 · 3 comments · Fixed by #1554
Assignees
Labels
bug Something isn't working

Comments

@ahadnagy
Copy link
Contributor

ahadnagy commented Aug 9, 2022

I've been trying to use async instruments, but got slightly confused about the API and the status of the implementation.

The problem I ran into is that the callbacks are not getting evaluated and no data is exported. I see that the API has recently changed in #1388 that introduced ObservableRegistry. It seems like the ObservableRegistry::Observe() method would be the one evaluating all the registered callbacks, but it doesn't get called anywhere. According to the corresponding issue (#1338), all the callbacks should be evaluated before/during collect().

Am I missing something here? I don't see any open issues or PRs for async instruments. If this is a bug, I'd glad to help out with a PR after some explanation about where and how this should happen.

Thanks a lot in advance!

@ahadnagy ahadnagy added the bug Something isn't working label Aug 9, 2022
@lalitb
Copy link
Member

lalitb commented Aug 9, 2022

Yes, thanks for raising the issue. I am aware of the issue and was planning for fix. But it you are already in process of debugging, please feel free to raise a PR. We need this before going for RC which is tentative planned for 15th Aug - https://github.com/open-telemetry/opentelemetry-cpp/milestone/13

@lalitb lalitb added this to the Metrics SDK - Release Candidate milestone Aug 9, 2022
@ahadnagy
Copy link
Contributor Author

ahadnagy commented Aug 9, 2022

What would be your preferred way of implementing this provided that AsyncMetricStorage doesn't know about ObservableRegistry and ObservableInstruments in general?
Giving AsyncMetricStorage a shrared_ptr to any of these doesn't seem very idiomatic.

@lalitb lalitb self-assigned this Aug 10, 2022
@lalitb
Copy link
Member

lalitb commented Aug 10, 2022

@ahadnagy - This is broken after #1495. For now, I have assigned it to myself, as I am responsible for breaking it :) I don't have the correct approach for now, let me go through the code once. Let me know if that is not fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants