Fix: 1712 - Validate Instrument meta data (name, unit, description) #1713

lalitb · 2022-10-26T07:00:46Z

Fixes #1712

Changes

Create valid instrument if the meta data is validated
Else, create noop-instrument.

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 · 2022-10-26T07:13:04Z

Codecov Report

Merging #1713 (c5ef46d) into main (e785f5f) will decrease coverage by 0.50%.
The diff coverage is 30.77%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1713      +/-   ##
==========================================
- Coverage   86.28%   85.79%   -0.49%     
==========================================
  Files         169      171       +2     
  Lines        5160     5212      +52     
==========================================
+ Hits         4452     4471      +19     
- Misses        708      741      +33

Impacted Files	Coverage Δ
...include/opentelemetry/sdk/metrics/view/predicate.h	`72.73% <ø> (ø)`
sdk/src/metrics/meter.cc	`45.79% <8.34%> (-10.37%)`	⬇️
sdk/include/opentelemetry/sdk/metrics/meter.h	`57.15% <57.15%> (ø)`
sdk/src/metrics/instrument_metadata_validator.cc	`100.00% <100.00%> (ø)`
ext/src/http/client/curl/http_client_curl.cc	`81.44% <0.00%> (+1.14%)`	⬆️

marcalff · 2022-10-26T08:13:05Z

sdk/src/metrics/meter.cc

+    OTEL_INTERNAL_LOG_ERROR("Meter::CreateUInt64Counter - failed. Invalid parameters."
+                            << name << " " << description << " " << unit);
+    return nostd::unique_ptr<metrics::Counter<uint64_t>>(
+        new metrics::NoopCounter<uint64_t>(name, description, unit));


The calling instrumented application will not notice the error if returning a noop counter.

Not crashing now with a noop is not helping.
Instead, bugs will be filed later against opentelemetry-cpp about instrument that produce no data.

How about returning a nullptr, forcing the calling application to do something about it ?

Yes, good point. I too thought about it, but also felt that making critical application crash for some issue in telemetry configuration is not a good idea. We can also be more explicit in error message stating that measurements won't be recorded.

This is also inline with the general error handling guidelines -

Whenever API call returns values that is expected to be non-null value - in case of error in processing logic - SDK MUST return a "no-op" or any other "default" object that was (ideally) pre-allocated and readily available. This way API call sites will not crash on attempts to access methods and properties of a null objects.

Thanks for the details. Ok with noop then.

ThomsonTan · 2022-10-26T18:19:46Z

api/include/opentelemetry/common/macros.h

@@ -90,6 +90,14 @@
 #  define OPENTELEMETRY_DEPRECATED_MESSAGE(msg)
 #endif

+// Regex support
+#if (__GNUC__ == 4 && (__GNUC_MINOR__ == 8 || __GNUC_MINOR__ == 9))


nit: GNUC_MINOR >= 8?

The regex support is partially implemented for gcc 4.8 and gcc 4.9 only. So the existing condition looks good to me.

But yeah, even GNUC_MINOR >= 8 should work fine. But let it be explicit. This was already existing in predicate.h, I just moved it here at common place :)

ThomsonTan · 2022-10-26T18:26:16Z

sdk/src/metrics/instrument_metadata_validator.cc

+#  if HAVE_WORKING_REGEX
+    : name_reg_key_{kInstrumentNamePattern}, unit_reg_key_
+{
+  kInstrumentUnitPattern


Was this line formatted by clang-format? The format looks a little weird.

yeah clang-format behaved weirdly here. I have disable the clang-format for this block of code. Looks better now.
Thanks for noticing.

ThomsonTan · 2022-10-26T18:27:16Z

sdk/src/metrics/instrument_metadata_validator.cc

+  kInstrumentUnitPattern
+}
+#  endif
+{}


Should this be put under #else block?

no it's fine actually. The part of code was somewhat confusing with clang-format. It should be more clear now.

ThomsonTan · 2022-10-26T18:43:15Z

sdk/src/metrics/instrument_metadata_validator.cc

+  {
+    return false;
+  }
+  // first char should be alphanumeric


First char should be alpha, not alphanumeric?

yeah good point. fixed the comment.

ThomsonTan · 2022-10-27T21:52:14Z

sdk/src/metrics/meter.cc

@@ -52,6 +61,14 @@ nostd::unique_ptr<metrics::Counter<double>> Meter::CreateDoubleCounter(
    nostd::string_view description,
    nostd::string_view unit) noexcept
 {
+  if (!ValidateInstrument(name, description, unit))
+  {
+    OTEL_INTERNAL_LOG_ERROR("Meter::CreateDoubleCounter - failed. Invalid parameters."


nit: probably use macro __func__ to avoid hard code function name in string liternal.

yes, probably we can modify OTEL_INTERNAL_* macros to automatically add class and method name to the logs. Will create a separate issue for this.

ThomsonTan · 2022-10-27T21:54:10Z

sdk/test/metrics/instrument_metadata_validator_test.cc

+  }
+}
+
+TEST(InstrumentMetadataValidator, TestUnit)


Missing unit test on description?

We don't have any validation checks for the description, so no tests added for it.

…ad-local-stack * commit '9acde87b08b225ce511fa8a20c6cba14f2921518': (36 commits) Prepare v1.7.0 release with Metrics API/SDK GA. (open-telemetry#1721) Fix: 1712 - Validate Instrument meta data (name, unit, description) (open-telemetry#1713) Document libthrift 0.12.0 doesn't work with Jaeger exporter (open-telemetry#1714) Fix:1674, Add Monotonic Property to Sum Aggregation, and unit tests for Up Down Counter (open-telemetry#1675) [Metrics SDK] Move Metrics Exemplar processing behind feature flag (open-telemetry#1710) [Metrics API/SDK] Change Meter API/SDK to return nostd::unique_ptr for Sync Instruments (open-telemetry#1707) [Metrics] Switch to explicit 64 bit integers (open-telemetry#1686) Add metrics e2e test to asan & tsan CI (open-telemetry#1670) Add otlp-grpc example bazel (open-telemetry#1708) [Metrics SDK] Add support for Pull Metric Reader (open-telemetry#1701) Fix debug log of OTLP HTTP exporter and ES log exporter (open-telemetry#1703) [SEMANTIC CONVENTIONS] Upgrade to version 1.14.0 (open-telemetry#1697) Fix a potential precision loss on integer in ReservoirCellIndexFor (open-telemetry#1696) fix Histogram crash (open-telemetry#1685) Fix:1676 Segfault when short export period is used for metrics (open-telemetry#1682) Add timeout support to MeterContext::ForceFlush (open-telemetry#1673) Add CMake OTELCPP_MAINTAINER_MODE (open-telemetry#1650) [DOCS] - Minor updates to OStream Metrics exporter documentation (open-telemetry#1679) Fix:open-telemetry#1575 API Documentation for Metrics SDK and API (open-telemetry#1678) Fixes open-telemetry#249 (open-telemetry#1677) ...

…pen-telemetry#1713)

lalitb and others added 2 commits October 25, 2022 23:37

validate instrument

05c0575

fix

6e0dda6

lalitb requested a review from a team October 26, 2022 07:00

fix build

f06d877

marcalff reviewed Oct 26, 2022

View reviewed changes

review comments, and build errors

5c42903

ThomsonTan reviewed Oct 26, 2022

View reviewed changes

disable clang

f9938cd

ThomsonTan reviewed Oct 26, 2022

View reviewed changes

lalitb and others added 6 commits October 26, 2022 11:59

fix gcc4.8 unit tests

0616147

fix

766c2dd

fix

cbfceae

fix

c191e69

fix more errors

c22da62

Merge branch 'main' into validate-instrument

c5ef46d

ThomsonTan reviewed Oct 27, 2022

View reviewed changes

ThomsonTan approved these changes Oct 27, 2022

View reviewed changes

lalitb merged commit dd7e257 into open-telemetry:main Oct 27, 2022

yxue pushed a commit to yxue/opentelemetry-cpp that referenced this pull request Dec 5, 2022

Fix: 1712 - Validate Instrument meta data (name, unit, description) (o…

f43b930

…pen-telemetry#1713)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: 1712 - Validate Instrument meta data (name, unit, description) #1713

Fix: 1712 - Validate Instrument meta data (name, unit, description) #1713

lalitb commented Oct 26, 2022

codecov bot commented Oct 26, 2022 •

edited

Loading

marcalff Oct 26, 2022

lalitb Oct 26, 2022 •

edited

Loading

marcalff Oct 26, 2022

ThomsonTan Oct 26, 2022

lalitb Oct 26, 2022

lalitb Oct 26, 2022 •

edited

Loading

ThomsonTan Oct 26, 2022

lalitb Oct 26, 2022

ThomsonTan Oct 26, 2022

lalitb Oct 26, 2022

ThomsonTan Oct 26, 2022

lalitb Oct 26, 2022

ThomsonTan Oct 27, 2022

lalitb Oct 27, 2022 •

edited

Loading

ThomsonTan Oct 27, 2022

lalitb Oct 27, 2022

Fix: 1712 - Validate Instrument meta data (name, unit, description) #1713

Fix: 1712 - Validate Instrument meta data (name, unit, description) #1713

Conversation

lalitb commented Oct 26, 2022

Changes

codecov bot commented Oct 26, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

lalitb Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalitb Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalitb Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 26, 2022 •

edited

Loading

lalitb Oct 26, 2022 •

edited

Loading

lalitb Oct 26, 2022 •

edited

Loading

lalitb Oct 27, 2022 •

edited

Loading