Fix: 1712 - Validate Instrument meta data (name, unit, description)#1713
Conversation
Codecov Report
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
|
| 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)); |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks for the details. Ok with noop then.
| #endif | ||
|
|
||
| // Regex support | ||
| #if (__GNUC__ == 4 && (__GNUC_MINOR__ == 8 || __GNUC_MINOR__ == 9)) |
There was a problem hiding this comment.
The regex support is partially implemented for gcc 4.8 and gcc 4.9 only. So the existing condition looks good to me.
There was a problem hiding this comment.
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 :)
| # if HAVE_WORKING_REGEX | ||
| : name_reg_key_{kInstrumentNamePattern}, unit_reg_key_ | ||
| { | ||
| kInstrumentUnitPattern |
There was a problem hiding this comment.
Was this line formatted by clang-format? The format looks a little weird.
There was a problem hiding this comment.
yeah clang-format behaved weirdly here. I have disable the clang-format for this block of code. Looks better now.
Thanks for noticing.
| kInstrumentUnitPattern | ||
| } | ||
| # endif | ||
| {} |
There was a problem hiding this comment.
Should this be put under #else block?
There was a problem hiding this comment.
no it's fine actually. The part of code was somewhat confusing with clang-format. It should be more clear now.
| { | ||
| return false; | ||
| } | ||
| // first char should be alphanumeric |
There was a problem hiding this comment.
First char should be alpha, not alphanumeric?
There was a problem hiding this comment.
yeah good point. fixed the comment.
| { | ||
| if (!ValidateInstrument(name, description, unit)) | ||
| { | ||
| OTEL_INTERNAL_LOG_ERROR("Meter::CreateDoubleCounter - failed. Invalid parameters." |
There was a problem hiding this comment.
nit: probably use macro __func__ to avoid hard code function name in string liternal.
There was a problem hiding this comment.
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.
| } | ||
| } | ||
|
|
||
| TEST(InstrumentMetadataValidator, TestUnit) |
There was a problem hiding this comment.
Missing unit test on description?
There was a problem hiding this comment.
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) ...
3 participants
Fixes #1712
Changes
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes