deps: update opentelemetry-cpp to 1.27.0 - #422
Conversation
WalkthroughBazel/CMake config updates, version bumps, and C++17 adoption. Extensive API/SDK changes: new metrics multi-observer callback API, TLS-configurable OTLP builders, Prometheus/ostream/OTLP builder libraries, configuration subsystem (YAML parser, types, env-driven options), ETW exporter tracing/timestamp tweaks, numerous semconv additions/updates, Zipkin builder removal, tests updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as Application
participant Meter as Meter (SDK)
participant Reg as ObservableRegistry
participant MR as MultiObserverResult
participant Inst as ObservableInstrument(s)
participant Reader as MetricReader
rect rgba(200, 230, 255, 0.5)
App->>Meter: RegisterCallback(cb, state, [Inst...])
Meter->>Reg: AddCallback(cb, state, [Inst...])
Reg->>MR: Register instruments
Reg-->>App: callback_id
end
rect rgba(200, 255, 200, 0.5)
Reader->>Meter: Collect()
Meter->>Reg: Observe()
Reg->>cb: Invoke with MR
cb->>MR: ForInstrument<T>().Observe(...)
Reg->>MR: StoreResults(collection_ts)
MR->>Inst: Write to storage
Reg-->>Meter: Done
Meter-->>Reader: Export metrics
end
rect rgba(255, 220, 200, 0.5)
App->>Meter: DeregisterCallback(callback_id)
Meter->>Reg: RemoveCallback(callback_id)
Reg-->>App: Ack
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
deps/opentelemetry-cpp/exporters/otlp/test/otlp_http_exporter_test.cc (1)
173-227:⚠️ Potential issue | 🟠 MajorFinish the HTTP callback on guard exits.
Line 180–199 returns early without finishing the callback, which can leave the request hanging and make
ForceFlush()flaky or block retries. Ensure the callback is always completed, even for guard failures.💡 Suggested fix (apply the same pattern to similar lambdas)
- if (check_json["resourceSpans"].size() == 0) - { - return; - } + auto finish = [&]() { + http_client::nosend::Response response; + response.Finish(*callback.get()); + }; + if (check_json["resourceSpans"].size() == 0) + { + finish(); + return; + } ... - if (received_trace_id != report_trace_id) - { - return; - } + if (received_trace_id != report_trace_id) + { + finish(); + return; + } ... - http_client::nosend::Response response; - response.Finish(*callback.get()); + finish();deps/opentelemetry-cpp/exporters/otlp/test/otlp_http_log_record_exporter_test.cc (1)
161-244:⚠️ Potential issue | 🟠 MajorFinish the HTTP callback on guard exits.
Line 168–181 returns early without finishing the callback, which can leave the request hanging and make
ForceFlush()flaky or block retries. Ensure the callback is always completed, even for guard failures.💡 Suggested fix (apply the same pattern to similar lambdas)
- if (check_json["resourceLogs"].size() == 0) - { - return; - } + auto finish = [&]() { + http_client::nosend::Response response; + response.Finish(*callback.get()); + }; + if (check_json["resourceLogs"].size() == 0) + { + finish(); + return; + } ... - if (received_trace_id == report_trace_id && received_span_id == report_span_id) - { - ++received_record_counter; - } + if (received_trace_id == report_trace_id && received_span_id == report_span_id) + { + ++received_record_counter; + } ... - http_client::nosend::Response response; - response.Finish(*callback.get()); + finish();deps/opentelemetry-cpp/exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h (1)
165-422:⚠️ Potential issue | 🟠 MajorValidate provider handle before marking tracer as open.
The constructor unconditionally sets
isClosed_tofalseafteretwProvider().open(provId, encoding)without checking whether the handle is valid. Sinceopen()can fail silently and return aHandlewithproviderHandle == INVALID_HANDLE(seeetw_provider.hlines 188–190 and 154, 172),IsClosed()will misreport the tracer's state. The destructor will subsequently attempt to close an invalid handle. CheckprovHandle.providerHandle != INVALID_HANDLEbefore settingisClosed_ = false, or handle registration failure appropriately in the constructor.deps/opentelemetry-cpp/api/include/opentelemetry/context/runtime_context.h (1)
244-343:⚠️ Potential issue | 🟠 MajorFix out-of-bounds read in
Contains()when size_ exceeds max_capacity_When
size_grows beyondcapacity_(which happens whensize_ > max_capacity_), theContains()method iterates fromsize_down to 1 and accessesbase_[pos - 1], reading past the allocated buffer. This occurs inDetach()when it callsContains()in deep-nesting scenarios. Clamp the search range to the actual allocated capacity:Proposed fix
bool Contains(const Token &token) const noexcept { - for (size_t pos = size_; pos > 0; --pos) + const size_t search_size = (std::min)(size_, capacity_); + for (size_t pos = search_size; pos > 0; --pos) { if (token == base_[pos - 1]) { return true; } } return false; }
🤖 Fix all issues with AI agents
In `@deps/opentelemetry-cpp/.bcr/presubmit.yml`:
- Around line 10-14: The CI presubmit uses a single verify_targets task that
sets build_flags['--cxxopt=-std=c++17'] for all platforms which breaks Windows
MSVC; split into two platform-specific verify tasks (e.g., verify_targets_unix
and verify_targets_windows) so the Unix-like matrix entries (debian10, macos,
ubuntu2004) keep build_flags with --cxxopt=-std=c++17 while the Windows entry
uses --cxxopt=/std:c++17; ensure each task still includes the same build_targets
(e.g., '@opentelemetry-cpp//api') and the matrix/when conditions select the
correct platforms.
In `@deps/opentelemetry-cpp/.devcontainer/Dockerfile.dev`:
- Around line 25-28: Replace the interactive update-alternatives calls with
non-interactive --set invocations: instead of running "update-alternatives
--config clang-tidy" and "update-alternatives --config llvm-config", use
"update-alternatives --set clang-tidy /usr/bin/clang-tidy-20" and
"update-alternatives --set llvm-config /usr/bin/llvm-config-20" so the Docker
build does not require a TTY; update the RUN line that currently calls
update-alternatives for clang-tidy and llvm-config accordingly.
In
`@deps/opentelemetry-cpp/api/include/opentelemetry/semconv/incubating/gcp_attributes.h`:
- Around line 101-107: The documentation comment above the constant
kGcpApphubDestinationServiceCriticalityType uses “destination workload” but the
constant is for destination service criticality; update the semconv
source/template to replace “destination workload” with “destination service” in
the docstring for kGcpApphubDestinationServiceCriticalityType (and any matching
incubating template entries), then regenerate the generated header so the
updated comment is emitted alongside that constant.
In
`@deps/opentelemetry-cpp/api/include/opentelemetry/semconv/incubating/system_metrics.h`:
- Around line 605-695: The deprecated metrics for
kMetricSystemLinuxMemoryAvailable and kMetricSystemLinuxMemorySlabUsage use
packet semantics and Counter types incorrectly; update
descrMetricSystemLinuxMemoryAvailable and descrMetricSystemLinuxMemorySlabUsage
to memory-appropriate text (e.g., "An estimate of how much memory is
available..." and "Reports the memory used by the slab allocator..."), change
unitMetricSystemLinuxMemoryAvailable and unitMetricSystemLinuxMemorySlabUsage to
"By", replace synchronous Counter types and factory calls in
CreateSyncInt64MetricSystemLinuxMemoryAvailable/CreateSyncDoubleMetricSystemLinuxMemoryAvailable
and
CreateSyncInt64MetricSystemLinuxMemorySlabUsage/CreateSyncDoubleMetricSystemLinuxMemorySlabUsage
to UpDownCounter versions (CreateInt64UpDownCounter/CreateDoubleUpDownCounter
and corresponding unique_ptr<metrics::UpDownCounter<...>> types), and replace
asynchronous ObservableCounter usage in
CreateAsyncInt64MetricSystemLinuxMemoryAvailable/CreateAsyncDoubleMetricSystemLinuxMemoryAvailable
and
CreateAsyncInt64MetricSystemLinuxMemorySlabUsage/CreateAsyncDoubleMetricSystemLinuxMemorySlabUsage
with ObservableUpDownCounter and
CreateInt64ObservableUpDownCounter/CreateDoubleObservableUpDownCounter to match
the non-deprecated memory semantics.
In
`@deps/opentelemetry-cpp/exporters/otlp/test/otlp_http_metric_exporter_test.cc`:
- Around line 287-293: The early-return branch that checks
request_body.resource_metrics_size()/scope_metrics_size()/metrics_size() exits
without completing the RPC; modify that branch to call response.Finish(...)
before returning (e.g., response.Finish(grpc::Status::OK) or the appropriate
Finish signature used elsewhere in this test) so the call's callback is always
invoked; update the block containing request_body and received_record_counter to
call response.Finish(...) then return.
- Around line 186-200: The early-return branches in the test (after checking
check_json["resourceMetrics"], resource_metrics["scopeMetrics"], and
scope_metrics["metrics"]) skip calling response.Finish(*callback.get()), which
can leave the HTTP client callback unresolved; update each early-return path in
otlp_http_metric_exporter_test.cc to call response.Finish(*callback.get())
before returning (or otherwise ensure the callback is completed), referencing
the existing response and callback objects used in the test so the HTTP client
is always signaled even when the JSON guards cause an early exit.
In
`@deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/configuration_parser.h`:
- Around line 374-377: Remove the unused member variable version_ from the
ConfigurationParser class declaration in configuration_parser.h: the file
currently declares version_, version_major_, and version_minor_, but only
version_major_ and version_minor_ are used/initialized; delete the unused
std::string version_ member to avoid dead code and keep the class consistent
with its implementation.
In
`@deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/configuration.h`:
- Line 70: Remove the erroneous use of the C-style `enum` keyword when declaring
the scoped enum variable: update the declaration of `log_level` to use the
scoped enum type `SeverityNumber` directly (replace `enum SeverityNumber
log_level = SeverityNumber::info;` with a declaration that omits `enum`) so the
variable `log_level` is correctly typed as `SeverityNumber` and initialized to
`SeverityNumber::info`.
In
`@deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/configured_sdk.h`:
- Around line 51-52: The member log_level in class ConfiguredSdk is left
uninitialized causing undefined behavior when Install() calls SetLogLevel();
modify the ConfiguredSdk() constructor to value-initialize log_level to
opentelemetry::sdk::common::internal_log::LogLevel::None so log_level has a
well-defined default before Install() reads it (leave resource initialization
as-is).
In `@deps/opentelemetry-cpp/sdk/src/CMakeLists.txt`:
- Around line 11-13: The CMake conditional USING the WITH_CONFIGURATION symbol
is never defined so the configuration subdirectory never builds; add an option
definition for WITH_CONFIGURATION in the parent CMakeLists so the flag can be
toggled (e.g., define a CMake option named WITH_CONFIGURATION with a sensible
default such as ON) and ensure that the three places that check
IF(WITH_CONFIGURATION) now respect that option; update the top-level or sdk
CMakeLists to declare the option before those IF(...) checks so
add_subdirectory(configuration) can be enabled when desired.
In `@deps/opentelemetry-cpp/sdk/src/configuration/CMakeLists.txt`:
- Around line 30-34: The pkg-config entry for the "configuration" component uses
the wrong description string; update the opentelemetry_add_pkgconfig call for
the component named "configuration" so the second description argument refers to
configuration (e.g., "Configuration components for the OpenTelemetry SDK" or
similar) rather than "Components for exporting traces in the OpenTelemetry SDK."
In `@deps/opentelemetry-cpp/sdk/src/trace/batch_span_processor_options.cc`:
- Around line 30-38: Both GetMaxQueueSizeFromEnv() and
GetMaxExportBatchSizeFromEnv() must guard against environment-parsed zero
values: after calling
opentelemetry::sdk::common::GetUintEnvironmentVariable(...) and before returning
the parsed value, check if the parsed uint value is zero and if so return
kDefaultMaxQueueSize (for GetMaxQueueSizeFromEnv) or kDefaultMaxExportBatchSize
(for GetMaxExportBatchSizeFromEnv); keep the existing fallback when
GetUintEnvironmentVariable fails, and otherwise return
static_cast<size_t>(value) only when value != 0.
🧹 Nitpick comments (11)
deps/opentelemetry-cpp/exporters/ostream/src/console_push_metric_builder.cc (1)
54-54: Nitpick: Trailing semicolon after switch block.Line 54 has a semicolon after the closing brace of the switch statement. While this is syntactically valid (empty statement), it's typically unnecessary. Since this is vendored upstream code, this is a very minor observation.
deps/opentelemetry-cpp/ext/test/http/curl_http_test.cc (1)
501-537: Make the TODO traceable to avoid it becoming permanent.Consider linking the TODO to the same issue referenced above so it’s easy to track re-enablement.
🔧 Proposed tweak
- // TODO: Spurious test failures here. + // TODO(`#3535`): Spurious test failures here.deps/opentelemetry-cpp/sdk/test/metrics/view_registry_test.cc (1)
96-138: Strengthen null-parameter tests to assert the view wasn’t registered.Right now these tests only check “no crash”; consider also asserting
FindViewsreturns the default view (or that the “test_view” name is never observed) to prove the call was ignored.deps/opentelemetry-cpp/.bcr/README.md (1)
14-16: Minor inconsistency in Bazel version notation.The presubmit.yml file uses
9.xbut this README says9.*. Consider aligning the notation for consistency.📝 Suggested fix
Defines the BCR presubmit tests that run when a new version is published. Currently configured to test on: - Platforms: debian10, macos, ubuntu2004, windows -- Bazel versions: 7.x, 8.x, 9.* +- Bazel versions: 7.x, 8.x, 9.xdeps/opentelemetry-cpp/sdk/src/metrics/state/observable_registry.cc (1)
64-132: Consider monotonic IDs to avoid stale-handle collisions.
Using the record pointer as the token can introduce ABA risk if callbacks are auto-removed and the allocator reuses the address; a monotonic counter avoids accidental removal of a new callback with a stale handle.deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/distribution_configuration.h (1)
7-7: Unused include.The
<string>header is included but not used in this file. The class only usesstd::vectorandstd::unique_ptr.Proposed fix
`#include` <memory> -#include <string> `#include` <vector>deps/opentelemetry-cpp/sdk/test/configuration/CMakeLists.txt (1)
16-19: Consider whethertrace.prefix is appropriate for all test types.The
TEST_PREFIX trace.is applied uniformly to all tests, includingyaml_logs_test,yaml_metrics_test, andyaml_distribution_test. This may cause confusion in test output where non-trace tests appear with atrace.prefix. If this is intentional for organizational reasons within the project, feel free to disregard.deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/document_node.h (1)
34-34: Default-initializeDocumentNodeLocationfields for consistency.The current implementation in
RymlDocument::Location()properly initializes all struct members before return. Adding default initialization with{}is optional defensive programming to ensure deterministic values across all potential implementations.Suggested improvement
class DocumentNodeLocation { public: - size_t offset; - size_t line; - size_t col; - std::string filename; + size_t offset{}; + size_t line{}; + size_t col{}; + std::string filename{}; std::string ToString() const; };deps/opentelemetry-cpp/sdk/test/configuration/yaml_distribution_test.cc (1)
65-103: Refactor test to use order-independent lookups instead of relying on iteration order.While RapidYAML preserves insertion order, YAML mappings are unordered by spec. Tests should not depend on iteration order to ensure compatibility if the parser changes and to follow best practices. Use
GetRequiredChildNode()to retrieve entries and properties by name:Order-independent approach
+ auto find_entry = [&](const char *name) { + for (auto &e : distribution->entries) + { + if (e && e->name == name) + return e.get(); + } + return static_cast<opentelemetry::sdk::configuration::DistributionEntryConfiguration *>(nullptr); + }; + - auto *entry_1 = distribution->entries[0].get(); + auto *entry_1 = find_entry("acme_vendor"); ASSERT_NE(entry_1, nullptr); ASSERT_EQ(entry_1->name, "acme_vendor"); node = entry_1->node.get(); ASSERT_NE(node, nullptr); - auto it = node->begin(); - property = (*it); - ASSERT_NE(property, nullptr); - name = property->Key(); - ASSERT_EQ(name, "a"); - ASSERT_EQ(property->AsInteger(), 12); - ++it; - property = (*it); - ASSERT_NE(property, nullptr); - name = property->Key(); - ASSERT_EQ(name, "b"); - ASSERT_EQ(property->AsInteger(), 34); - ++it; - property = (*it); - ASSERT_NE(property, nullptr); - name = property->Key(); - ASSERT_EQ(name, "c"); - ASSERT_EQ(property->AsInteger(), 56); - ++it; - ASSERT_EQ(node->end(), it); + auto a = node->GetRequiredChildNode("a"); + auto b = node->GetRequiredChildNode("b"); + auto c = node->GetRequiredChildNode("c"); + ASSERT_NE(a, nullptr); + ASSERT_NE(b, nullptr); + ASSERT_NE(c, nullptr); + ASSERT_EQ(a->AsInteger(), 12); + ASSERT_EQ(b->AsInteger(), 34); + ASSERT_EQ(c->AsInteger(), 56); - auto *entry_2 = distribution->entries[1].get(); + auto *entry_2 = find_entry("other_vendor"); ASSERT_NE(entry_2, nullptr); ASSERT_EQ(entry_2->name, "other_vendor");deps/opentelemetry-cpp/sdk/test/metrics/multi_observer_test.cc (1)
124-129: Use explicit span data/size to avoid constructor ambiguity.
nostd::spanmay not acceptstd::vectordirectly across all configurations; usingdata()/size()keeps it consistent with earlier usage.♻️ Suggested tweak
- auto callback_id = - meter->RegisterCallback(empty_multi_observable_callback, &was_called, - nostd::span<metrics::ObservableInstrument *>{empty_instruments}); + auto callback_id = + meter->RegisterCallback( + empty_multi_observable_callback, &was_called, + nostd::span<metrics::ObservableInstrument *>{empty_instruments.data(), + empty_instruments.size()});deps/opentelemetry-cpp/sdk/src/configuration/configuration_parser.cc (1)
135-261: Large enum-to-string mapping could benefit from a lookup table.The
ParseSeverityNumberfunction uses a long chain of if-else comparisons for 24 severity levels. While functionally correct, this pattern is verbose.However, since this is vendored upstream code from opentelemetry-cpp, modifying it would create maintenance burden during future updates.
|
Adding to PRs Ready for Deployment and moving to Icebox since this PR has been under this pipeline for more than 50 days. Once ready to merge please make sure to pull it back in the sprint and make sure it's associated with a ticket. |
221754d to
b038d06
Compare
fe70b83 to
46e6d9f
Compare
46e6d9f to
25c4994
Compare
santigimeno
changed the title
25c4994 to
5b63c75
Compare
|
@coderabbitai you should not be reviewing dep PR's. Approve |
|
✏️ Learnings added
✅ Actions performedComments resolved and changes approved. |
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Changes