[WIP] Synchronous Gauge #2341

lalitb · 2023-09-28T07:16:19Z

Fixes #2279 , #2155

Changes

Please provide a brief description of the changes here.

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

codecov · 2023-09-29T05:41:59Z

Codecov Report

Merging #2341 (1248928) into main (cb603ad) will decrease coverage by 0.39%.
The diff coverage is 55.69%.

❗ Current head 1248928 differs from pull request most recent head 272f30f. Consider uploading reports for the commit 272f30f to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2341      +/-   ##
==========================================
- Coverage   87.41%   87.02%   -0.39%     
==========================================
  Files         199      199              
  Lines        6028     6036       +8     
==========================================
- Hits         5269     5252      -17     
- Misses        759      784      +25

Files	Coverage Δ
api/include/opentelemetry/metrics/meter.h	`100.00% <ø> (ø)`
api/include/opentelemetry/metrics/noop.h	`41.18% <ø> (+3.93%)`	⬆️
...i/include/opentelemetry/metrics/sync_instruments.h	`100.00% <100.00%> (ø)`
sdk/include/opentelemetry/sdk/metrics/meter.h	`57.15% <ø> (ø)`
...clude/opentelemetry/sdk/metrics/sync_instruments.h	`100.00% <ø> (+19.52%)`	⬆️
sdk/src/metrics/meter.cc	`61.45% <100.00%> (ø)`
...etry/sdk/metrics/aggregation/default_aggregation.h	`70.13% <0.00%> (ø)`
sdk/src/metrics/sync_instruments.cc	`65.84% <53.09%> (-13.05%)`	⬇️

... and 14 files with indirect coverage changes

marcalff · 2023-09-29T09:41:03Z

sdk/include/opentelemetry/sdk/metrics/instruments.h

+#if OPENTELEMETRY_ABI_VERSION_NO >= 2
+  kGauge,
+#endif


The SDK can implement more features like kGauge even when these are not reachable from ABI v1, because there is no CreateIntGauge() virtual method in the API for ABI V1.

So, it should be just fine to always declare kGauge here, without if OPENTELEMETRY_ABI_VERSION_NO >= 2.

Ideally, in the SDK, testing for OPENTELEMETRY_ABI_VERSION_NO should be only needed when overriding a virtual method, limiting the amount of ifdef to a minimum.

Good point. Have removed the OPENTELEMETRY_ABI_VERSION_NO from SDK where possible.

Should we append kGuage at the end of the enum so other values are still backward compatible?

marcalff · 2023-09-29T09:58:40Z

api/include/opentelemetry/metrics/sync_instruments.h

+  /**
+   * Record a value
+   *
+   * @param value The increment amount. MUST be non-negative.
+   */
+  virtual void Record(T value) noexcept = 0;
+
+  /**
+   * Record a value
+   *
+   * @param value The increment amount. MUST be non-negative.
+   * @param context The explicit context to associate with this measurement.
+   */
+  virtual void Record(T value, const context::Context &context) noexcept = 0;
+
+  /**
+   * Record a value with a set of attributes.
+   *
+   * @param value The increment amount. MUST be non-negative.
+   * @param attributes A set of attributes to associate with the value.
+   */
+
+  virtual void Record(T value, const common::KeyValueIterable &attributes) noexcept = 0;
+
+  /**
+   * Record a value with a set of attributes.
+   *
+   * @param value The increment amount. MUST be non-negative.
+   * @param attributes A set of attributes to associate with the value.
+   * @param context The explicit context to associate with this measurement.
+   */
+  virtual void Record(T value,
+                      const common::KeyValueIterable &attributes,
+                      const context::Context &context) noexcept = 0;


Here there are 4 virtual methods for the ABI, with different parameters combinations.

An alternative is to define only one virtual method for the ABI, accepting pointers instead of references:

virtual void Record(T value, const common::KeyValueIterable *attributes, const context::Context *context) noexcept = 0;

and then have all the API helpers invoke this unique entry point, passing nullptr for optional parameters.

We can still have a helper like:

void Record(T value, const common::KeyValueIterable &attributes, const context::Context &context) { this->Record(value, &attributes, &context); }

so that user code pass references as usual.

Making a clear distinction between the ABI (the virtual method) and the API helpers (non virtual) will help,
because it is then easier to decide if a change is a breaking change or not.

Changes to the unique ABI virtual method is a breaking change.

Changes to the plenty of API helpers accepting combinations of parameters is not breaking the ABI,
as it always forwards to the same prototype.

A much cleaner method! Thanks for suggesting. I saw this approach in the GetMeter() PR but overlooked adding it here. Done now :)

marcalff

Thanks for taking on the implementation for Gauge.

A few early comments.

lalitb · 2023-09-29T17:31:49Z

Thanks for taking on the implementation for Gauge.

A few early comments.

Thanks for the comments. Need to add unit-tests before making it ready now.

marcalff · 2023-10-02T06:56:44Z

sdk/src/metrics/sync_instruments.cc

+                           << instrument_descriptor_.name_);
+    return;
+  }
+  return storage_->RecordLong(value, *attributes, *context);


Keep in mind attributes and context can be nullptr here.

Call the proper RecordLong() method, and/or use:

auto context = opentelemetry::context::Context{};

ThomsonTan · 2023-11-21T00:49:18Z

api/include/opentelemetry/metrics/meter.h

+  /**
+   * Creates a Gauge with the passed characteristics and returns a
+   * unique_ptr to that Gauge
+   *  @since ABI_VERSION 2


Align with below @param?

vihangm · 2024-01-02T22:24:29Z

sdk/src/metrics/meter.cc

+  }
+  InstrumentDescriptor instrument_descriptor = {
+      std::string{name.data(), name.size()}, std::string{description.data(), description.size()},
+      std::string{unit.data(), unit.size()}, InstrumentType::kGauge, InstrumentValueType::kLong};


there's a small bug here, the instrument value type should be kDouble

lalitb · 2024-02-21T17:43:19Z

closing as there needs a design change. Will raise a new PR.

yuma-m · 2024-07-30T08:19:53Z

closing as there needs a design change. Will raise a new PR.

Hello @lalitb, Do you have any plan or timeline to add synchronous gauge support for OpenTelemetry SDK C++?

lalitb · 2024-08-02T17:31:08Z

Hello @lalitb, Do you have any plan or timeline to add synchronous gauge support for OpenTelemetry SDK C++?

Sorry, no timeline yet. It may take a couple of months for me to get back to it. If someone picks it up meanwhile, it would be great.

yuma-m · 2024-08-04T10:42:18Z

Sorry, no timeline yet. It may take a couple of months for me to get back to it. If someone picks it up meanwhile, it would be great.

@lalitb, Thank you for your answer, got it. As you've closed this as there needs a design change, are there any design documents to refer to?

initial commit

2e1e696

lalitb requested a review from a team September 28, 2023 07:16

lalitb marked this pull request as draft September 28, 2023 07:16

lalitb added 2 commits September 28, 2023 10:07

Merge branch 'main' into sync-gauge

f5c3c80

changes

6ac46fb

lalitb added 5 commits September 28, 2023 22:45

validate double value

b6909af

compile error

c511147

compile error

b9f538b

msvc build

06cd640

add tests..

9e34355

marcalff reviewed Sep 29, 2023

View reviewed changes

review comments

4134c00

lalitb and others added 6 commits September 29, 2023 16:56

fix test

e0bdccc

add test

127b107

fix compile error

e6abe54

Merge branch 'main' into sync-gauge

a3c0101

fix unused variable

03c3cab

more unused vars

1248928

marcalff reviewed Oct 2, 2023

View reviewed changes

ThomsonTan reviewed Nov 21, 2023

View reviewed changes

Merge branch 'main' into sync-gauge

272f30f

ThomsonTan mentioned this pull request Nov 29, 2023

[Metrics] Make context optional for histogram instruments in Metrics SDK #2416

Merged

3 tasks

vihangm mentioned this pull request Dec 12, 2023

Sync gauge gimletlabs/opentelemetry-cpp#1

Merged

vihangm reviewed Jan 2, 2024

View reviewed changes

lalitb closed this Feb 21, 2024

lalitb mentioned this pull request Aug 26, 2024

[API] Add synchronous gauge #3029

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Synchronous Gauge #2341

[WIP] Synchronous Gauge #2341

lalitb commented Sep 28, 2023 •

edited

Loading

codecov bot commented Sep 29, 2023 •

edited

Loading

marcalff Sep 29, 2023 •

edited

Loading

lalitb Sep 29, 2023

ThomsonTan Nov 20, 2023

marcalff Sep 29, 2023 •

edited

Loading

lalitb Sep 29, 2023 •

edited

Loading

marcalff left a comment

lalitb commented Sep 29, 2023

marcalff Oct 2, 2023

ThomsonTan Nov 21, 2023

vihangm Jan 2, 2024

lalitb commented Feb 21, 2024

yuma-m commented Jul 30, 2024

lalitb commented Aug 2, 2024

yuma-m commented Aug 4, 2024

[WIP] Synchronous Gauge #2341

[WIP] Synchronous Gauge #2341

Conversation

lalitb commented Sep 28, 2023 • edited Loading

Changes

codecov bot commented Sep 29, 2023 • edited Loading

Codecov Report

marcalff Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

lalitb Sep 29, 2023

Choose a reason for hiding this comment

ThomsonTan Nov 20, 2023

Choose a reason for hiding this comment

marcalff Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

lalitb Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

marcalff left a comment

Choose a reason for hiding this comment

lalitb commented Sep 29, 2023

marcalff Oct 2, 2023

Choose a reason for hiding this comment

ThomsonTan Nov 21, 2023

Choose a reason for hiding this comment

vihangm Jan 2, 2024

Choose a reason for hiding this comment

lalitb commented Feb 21, 2024

yuma-m commented Jul 30, 2024

lalitb commented Aug 2, 2024

yuma-m commented Aug 4, 2024

lalitb commented Sep 28, 2023 •

edited

Loading

codecov bot commented Sep 29, 2023 •

edited

Loading

marcalff Sep 29, 2023 •

edited

Loading

marcalff Sep 29, 2023 •

edited

Loading

lalitb Sep 29, 2023 •

edited

Loading