Add synchronous gauge

[uuzay]

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
• [x] Unit tests have been added
• [ ] Changes in public API reviewed

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.88%. Comparing base (497eaf4) to head (56bdf77). Report is 151 commits behind head on main.

Files with missing lines	Patch %	Lines
sdk/src/metrics/state/metric_collector.cc	60.00%	2 Missing :warning:
...etry/sdk/metrics/aggregation/default_aggregation.h	0.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3029      +/-   ##
==========================================
+ Coverage   87.12%   87.88%   +0.76%     
==========================================
  Files         200      195       -5     
  Lines        6109     6137      +28     
==========================================
+ Hits         5322     5393      +71     
+ Misses        787      744      -43     

Files with missing lines	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% <ø> (ø)
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%)	:arrow_up:
sdk/src/metrics/sync_instruments.cc	65.84% <ø> (ø)
...etry/sdk/metrics/aggregation/default_aggregation.h	70.13% <0.00%> (ø)
sdk/src/metrics/state/metric_collector.cc	87.10% <60.00%> (-5.76%)	:arrow_down:

... and 94 files with indirect coverage changes

[lalitb]

@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?

[punya]

@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]

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]

@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]

@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.

[uuzay]

Hi @lalitb, Sorry for the delays, I was both on a holiday period and also had other important tasks to take care of.

I'm planning to make the necessary changes regarding the ABI macros this week. For delta temporality, I think what you suggested makes sense for now.

[netlify[bot]]

✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name	Link
🔨 Latest commit	56bdf77f7ebc0099aea2a3e0cf50d489ca83d39b
🔍 Latest deploy log	https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/67229beb935b030008bac3ad

[uuzay]

Hi @lalitb, I'm trying to figure out where would be an appropriate place to throw the error and would appreciate some pointers.

I was thinking if SyncMetricStorage or TemporalMetricStorage constructor would be a good place. Another idea that comes to mind is modifying DeltaTemporalitySelector in otlp_metric_utils to send a warning log and select cumulative temporality instead but I'm not sure if this covers all of the cases.

Thanks in advance.

[lalitb]

Hi @lalitb, I'm trying to figure out where would be an appropriate place to throw the error and would appreciate some pointers.

I was thinking if SyncMetricStorage or TemporalMetricStorage constructor would be a good place. Another idea that comes to mind is modifying DeltaTemporalitySelector in otlp_metric_utils to send a warning log and select cumulative temporality instead but I'm not sure if this covers all of the cases.

Thanks in advance.

@uuzay Yes that should work. Another easy place could be MetricCollector::GetAggregationTemporality. This part of code is called for both periodic reader and Prometheus, so should cover all exporters, and also in future when we support Delta for Prometheus.

[marcalff]

@uuzay Please merge a recent main into this PR, to get the merge conflict resolved. This should force the CI to build also, somehow it got stuck. Thanks.

[ThomsonTan]

Apologize for blocking the PR. I think this is the exactly the issue I raised before.

The user application builds fine with either new SDK headers/old SDK libs, or old SDK headers/new SDK libs, either one will trigger a runtime break which is very hard to investigate like memory corruption.

It is true that the SDK doesn't guarantee ABI compatibility, but we don't need break this if not necessary. In this specific case, either wrap the new kGauge in the ABI version 2 macro, or we can just append it to the end of the enum list, so we don't trigger an potential break.

Even it is documented that SDK doesn't support ABI combability, it may be ignored by common users as this actually works in most of the time. A better solution will be that we check the SDK version and SDK lib version, and raise error (like block linking) if both versions mismatch.

How to you think?

[marcalff]

The user application builds fine with either new SDK headers/old SDK libs, or old SDK headers/new SDK libs, either one will trigger a runtime break which is very hard to investigate like memory corruption.

The only way for this to happen is to link dynamically with a shared library/DLL for the SDK itself, using a library from a different version compared to header files used during the application build.

Any application compiling the with the SDK header files and linking statically with the matching SDK library will be just fine.

Wrapping kGauge with ifdef causes another list of issues as well, because user code may break as well if not using the same ifdef.

I think that we can move kGauge at the end of the enum, to minimize binary compatibility issues and in practice avoid breaks for this case alone.

[marcalff]

I think that we can move kGauge at the end of the enum, to minimize binary compatibility issues and in practice avoid breaks for this case alone.

Implemented as code review comment.

@ThomsonTan PTAL.

[ThomsonTan]

The user application builds fine with either new SDK headers/old SDK libs, or old SDK headers/new SDK libs, either one will trigger a runtime break which is very hard to investigate like memory corruption.

The only way for this to happen is to link dynamically with a shared library/DLL for the SDK itself, using a library from a different version compared to header files used during the application build.

Any application compiling the with the SDK header files and linking statically with the matching SDK library will be just fine.

Wrapping kGauge with ifdef causes another list of issues as well, because user code may break as well if not using the same ifdef.

I think that we can move kGauge at the end of the enum, to minimize binary compatibility issues and in practice avoid breaks for this case alone.

Looks good to me then, thanks.