Fix handling of empty metric collection cycles by ocelotl · Pull Request #3335 · open-telemetry/opentelemetry-python

ocelotl · 2023-06-05T15:25:47Z

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py

aabmass

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, 
        
           )

ocelotl · 2023-08-17T15:36:27Z

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,

)

Right, fixed.

ocelotl · 2023-08-18T12:50:39Z

I am investigating #3277, marking this as a draft meanwhile

aabmass

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.

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_consumer.py

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py

opentelemetry-sdk/tests/metrics/integration_test/test_histogram_export.py

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py

opentelemetry-sdk/tests/metrics/integration_test/test_histogram_export.py

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_consumer.py

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py

Fixes open-telemetry#3198

This reverts commit 3407110.

Fixes open-telemetry#3198

This reverts commit 1ea9c69.

ocelotl requested a review from a team June 5, 2023 15:25

ocelotl self-assigned this Jun 5, 2023

ocelotl force-pushed the issue_3198 branch from 768b28c to 3792fa4 Compare June 5, 2023 15:26

shalevr approved these changes Jun 6, 2023

View reviewed changes

aabmass reviewed Jun 23, 2023

View reviewed changes

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py Outdated Show resolved Hide resolved

aabmass requested changes Jun 23, 2023

View reviewed changes

srikanthccv mentioned this pull request Jul 30, 2023

Don't collect empty metrics from storage #3395

Closed

7 tasks

ocelotl force-pushed the issue_3198 branch 4 times, most recently from 5e795f5 to de1db4d Compare August 17, 2023 15:20

ocelotl requested a review from aabmass August 17, 2023 15:22

ocelotl force-pushed the issue_3198 branch from 56dce38 to 389cb7c Compare August 17, 2023 19:48

ocelotl marked this pull request as draft August 18, 2023 12:50

aabmass reviewed Aug 18, 2023

View reviewed changes

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_consumer.py Show resolved Hide resolved

aabmass self-requested a review August 18, 2023 23:06

ocelotl marked this pull request as ready for review August 21, 2023 14:58

ocelotl mentioned this pull request Aug 21, 2023

ExplicitBucketHistogram resets counts to zero even if aggregation temporality is cumulative #3407

Closed

ocelotl force-pushed the issue_3198 branch from f50c59a to 193aa0a Compare August 21, 2023 15:10

aabmass reviewed Aug 23, 2023

View reviewed changes

ocelotl changed the title ~~Remove console exporter metric output if empty~~ Fix handling of empty metric collection cycles Aug 24, 2023

ocelotl requested a review from aabmass August 24, 2023 16:06

ocelotl mentioned this pull request Aug 30, 2023

Several bugs with metrics aggregation #3418

Closed

ocelotl commented Aug 30, 2023

View reviewed changes

ocelotl mentioned this pull request Sep 6, 2023

Fix SumAggregation #3390

Merged

ocelotl force-pushed the issue_3198 branch from f398765 to e4b44ae Compare September 6, 2023 14:24

aabmass reviewed Sep 6, 2023

View reviewed changes

ocelotl force-pushed the issue_3198 branch from 5ab181e to e87c2c1 Compare September 7, 2023 08:37

ocelotl requested a review from aabmass September 7, 2023 09:03

ocelotl mentioned this pull request Sep 7, 2023

Histograms are exported empty if no observations are recorded during a reading interval #3277

Closed

lzchen approved these changes Sep 7, 2023

View reviewed changes

aabmass approved these changes Sep 7, 2023

View reviewed changes

ocelotl added 14 commits September 7, 2023 20:22

Remove console exporter metric output if empty

3d684b5

Fixes open-telemetry#3198

Revert "Remove console exporter metric output if empty"

c55f8fa

This reverts commit 3407110.

Speed up test execution

8c2a43a

Do not produce any metric output without metric data

a54b100

Fixes open-telemetry#3198

Revert "Speed up test execution"

1aeaf0c

This reverts commit 1ea9c69.

Fix lint

032e98f

Add another test for histograms

c731316

Remove added spacing

7ffc2b5

Update typing

721d012

Fix typing for collect

d9a078c

Fix histogram test

401cba9

Make return more concise

0aafae5

Move return inside the lock

cb97ad3

Add explanatory comment

58d1216

ocelotl force-pushed the issue_3198 branch from cdd760b to 58d1216 Compare September 7, 2023 18:24

ocelotl merged commit 3a651c7 into open-telemetry:main Sep 7, 2023

ocelotl deleted the issue_3198 branch September 8, 2023 09:26

ocelotl mentioned this pull request Nov 2, 2023

urllib instrumentation failing after empty metric collection handling fix open-telemetry/opentelemetry-python-contrib#2032

Closed

	self._receive_metrics(
	self._collect(self, timeout_millis=timeout_millis),
	timeout_millis=timeout_millis,
	)

Conversation

ocelotl commented Jun 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

aabmass left a comment

Choose a reason for hiding this comment

Uh oh!

ocelotl commented Aug 17, 2023

Uh oh!

ocelotl commented Aug 18, 2023

Uh oh!

aabmass left a comment • edited by ocelotl Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ocelotl commented Jun 5, 2023 •

edited

Loading

aabmass left a comment •

edited by ocelotl

Loading