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

Add synchronous gauge #3029

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

uuzay
Copy link

@uuzay uuzay commented Aug 14, 2024

Fixes #2279

Changes

Added synchronous gauge implementation according to the specification.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@uuzay uuzay requested a review from a team August 14, 2024 16:42
Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.60%. Comparing base (497eaf4) to head (bd8cb23).
Report is 117 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3029      +/-   ##
==========================================
+ Coverage   87.12%   87.60%   +0.49%     
==========================================
  Files         200      190      -10     
  Lines        6109     5870     -239     
==========================================
- Hits         5322     5142     -180     
+ Misses        787      728      -59     
Files Coverage Δ
api/include/opentelemetry/metrics/meter.h 100.00% <ø> (ø)
api/include/opentelemetry/metrics/noop.h 41.18% <ø> (ø)
...i/include/opentelemetry/metrics/sync_instruments.h 100.00% <ø> (ø)
...etry/sdk/metrics/aggregation/default_aggregation.h 70.13% <ø> (ø)
sdk/include/opentelemetry/sdk/metrics/meter.h 57.15% <ø> (ø)
...clude/opentelemetry/sdk/metrics/sync_instruments.h 100.00% <ø> (ø)
sdk/src/metrics/meter.cc 62.36% <ø> (+0.91%) ⬆️
sdk/src/metrics/sync_instruments.cc 65.84% <ø> (ø)

... and 118 files with indirect coverage changes

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feature.

This is not a full review yet,
only some preliminary comments.

Please fix:

  • declarations of new virtual methods using ABI version 2
  • build break issues found in CI

More extensive review to follow.

api/include/opentelemetry/metrics/meter.h Show resolved Hide resolved
@uuzay uuzay requested a review from marcalff August 20, 2024 13:16
Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feature.

The code structure looks good to me overall.

I defer to @lalitb to check the logic around aggregation temporality in particular,
as I am not too familiar with this area.

Comment on lines +22 to +24
#if OPENTELEMETRY_ABI_VERSION_NO >= 2
kGauge,
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be simpler to just declare this enum value all the time, and not conditionally.

The sdk headers can change, and having kGauge declared even in ABI v1 is not disruptive.

It will just be dead code in ABI v1.

Please adjust the switch/cases accordingly, to avoid the ifdef

@lalitb
Copy link
Member

lalitb commented Aug 26, 2024

@uuzay Thanks for the PR. Had a quick glance to the code, this looks similar to #2341 (correct me if not). And I believe this won't work for delta temporality. To validate, can we update the PR to test for it?


INSTANTIATE_TEST_SUITE_P(WritableMetricStorageTestDouble,
WritableMetricStorageTestFixture,
::testing::Values(AggregationTemporality::kCumulative));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the test here to include the delta temporality.

@punya
Copy link
Member

punya commented Aug 30, 2024

@uuzay Thanks for the PR. Had a quick glance to the code, this looks similar to #2341 (correct me if not). And I believe this won't work for delta temporality. To validate, can we update the PR to test for it?

If I understand the relevant section of the spec correctly, Gauges (unlike UpDownCounters and Histograms) don't support a choice of temporality.

@uuzay
Copy link
Author

uuzay commented Sep 5, 2024

I was thinking the same @punya.

It also mentions:

Gauges do not provide an aggregation semantic, instead “last sample value” is used when performing operations like temporal alignment or adjusting resolution. (source)

if that's related.

@lalitb can you please clarify if delta temporality is actually needed?

@lalitb
Copy link
Member

lalitb commented Sep 5, 2024

@lalitb can you please clarify if delta temporality is actually needed?

Sorry i was not clear enough. I meant testing for temporality with last value aggregation. The current test is only doing for cumulative.

@lalitb
Copy link
Member

lalitb commented Oct 3, 2024

@uuzay - Do you think we can modify the PR to restrict configuring the delta temporality for Last Value as of now - Just thrown an error at the startup if this is enabled through View or otherwise. With that, we should be good to go through this PR.

Thanks for working on this, and sorry about the long wait on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Synchronous Gauge instrument to the metrics API/SDK
5 participants