-
Notifications
You must be signed in to change notification settings - Fork 626
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
Fix handling of empty metric collection cycles #3335
Conversation
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should discuss the issue more in the SIG.
This PR would be a breaking change of the API. If we wanted to go this route, I'd recommend changing here to not call self._receive_metrics
if they are empty instead of making it Optional
:
opentelemetry-python/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py
Lines 325 to 328 in 9d3d0f8
self._receive_metrics( | |
self._collect(self, timeout_millis=timeout_millis), | |
timeout_millis=timeout_millis, | |
) |
5e795f5
to
de1db4d
Compare
Right, fixed. |
I am investigating #3277, marking this as a draft meanwhile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this might help for deltas, the SDK should be returning metrics every single collection for readers that are CUMULATIVE (the default). However that seems to not be working corrctly #3277 (comment)
Yes, noticed that now that I added another test case. I think this is a problem for the explicit bucket histogram aggregation who is resetting to zero all bucket counts after every collection even in the aggregation temporality is cumulative. I say we review this PR and address that issue separately.
Fixes #3198