deps: update to opentelemetry 1.23.0 #371
Conversation
PR-URL: #180 Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-by: Trevor Norris <[email protected]>
WalkthroughUpdates OpenTelemetry C++ vendored code to 1.23.0 with dependency bumps, extensive semconv additions/edits, new builders/registry-based SDK configuration, new exporter builder classes (OTLP/console/Zipkin/Prometheus), SDK metrics cardinality and view API changes, internal processor shutdown refactors, HTTP/cURL adjustments, and broad test/CMake updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Registry as sdk::configuration::Registry
participant SdkBuilder as sdk::configuration::SdkBuilder
participant Config as sdk::configuration::Configuration
participant Providers as {Tracer/Meter/Logger Providers}
participant Prop as TextMapPropagator
participant Exporters as {Span/Metric/Log Exporters}
App->>Registry: Construct / customize builders (optional)
App->>SdkBuilder: new SdkBuilder(shared_ptr<Registry>)
App->>SdkBuilder: CreateConfiguredSdk(Config*)
Note over SdkBuilder: Visits config trees and builds components
SdkBuilder->>Exporters: Use registered builders to create exporters
SdkBuilder->>Providers: Create providers with processors/readers
SdkBuilder->>Prop: Build composite propagator (composite/composite_list)
SdkBuilder-->>App: std::unique_ptr<ConfiguredSdk>
App->>Providers: sdk->Install() sets globals
Note over Providers,Prop: Global singletons set if present
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 48
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
deps/opentelemetry-cpp/api/include/opentelemetry/semconv/incubating/k8s_metrics.h (1)
294-303: Fix multi-line string literals breaking compilationThe updated description now spans multiple physical lines without closing/reopening the string literal. This injects raw newlines into the literal (e.g., Line 295) and yields a compile-time
missing terminating " charactererror. I’m seeing the same pattern in other new descriptions added in this header. Please regenerate or manually split these descriptions into concatenated string literals so no raw newline appears inside the quotes.deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h (1)
8-34: Avoid aggregation↔state header cyclePulling
kAggregationCardinalityLimitfromattributes_hashmap.hmakes this public aggregation header depend on the state layer. Unfortunately,attributes_hashmap.halready includesaggregation/aggregation_config.h, so this introduce a circular include that will break compilation under a clean rebuild. Please break the cycle—e.g., move the limit constant to a lightweight header that both files can include, or forward‑declare it in a config header without dragging in the entire state implementation.deps/opentelemetry-cpp/sdk/src/logs/multi_log_record_processor.cc (1)
137-144: Do not OR the shutdown results — this masks exporter failures.Line 139 uses
result |= processor->Shutdown(timeout);whileresultwas initialized totrue.true |= falsestaystrue, so the method now always reports success even if any underlying processor fails to shut down. That’s a regression in error propagation. Please combine the results with logical/bitwise AND instead so a single failure bubbles up:- result |= processor->Shutdown(timeout); + result &= processor->Shutdown(timeout);deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/trace/samplers/parent.h (1)
66-69: Fix outdated doc reference to delegate_sampler_.Comment still mentions delegate_sampler_. Update to reflect root_sampler_.
- * @return Description MUST be ParentBased{delegate_sampler_.getDescription()} + * @return Description MUST be ParentBased{root_sampler_->GetDescription()}deps/opentelemetry-cpp/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_grpc_client.h (1)
169-173: Fix comment typoNits in public header doc.
Apply this diff:
- /** - * Reomve reference fro a guard object + /** + * Remove reference from a guard objectdeps/opentelemetry-cpp/sdk/src/configuration/configuration_parser.cc (1)
697-705: Support legacy “prometheus” config key
In deps/opentelemetry-cpp/sdk/src/configuration/configuration_parser.cc (lines 697–705), allowname == "prometheus"alongside"prometheus/development":- if (name == "prometheus/development") + if (name == "prometheus/development" || name == "prometheus")
🧹 Nitpick comments (88)
deps/opentelemetry-cpp/sdk/src/trace/samplers/trace_id_ratio.cc (1)
59-65: Big-endian reconstruction fixes host-endian bias; minor clarity tweakThe big-endian loop ensures deterministic sampling across architectures. For clarity and to avoid raw pointers/implicit promotions, iterate the span and use size_t with an explicit cast.
Apply this diff:
- // Always interpret as big-endian - const uint8_t *data = trace_id.Id().data(); - uint64_t res = 0; - for (int i = 0; i < 8; ++i) - { - res = (res << 8) | data[i]; - } + // Always interpret as big-endian + auto id = trace_id.Id(); + uint64_t res = 0; + for (size_t i = 0; i < 8; ++i) + { + res = (res << 8) | static_cast<uint64_t>(id[i]); + }deps/opentelemetry-cpp/api/include/opentelemetry/semconv/incubating/container_attributes.h (1)
140-155: LGTM: New runtime attributes (description/name/version).Additive API; aligns with semconv split of runtime metadata. Consider migrating instrumentation to populate these keys.
deps/opentelemetry-cpp/sdk/test/metrics/sum_aggregation_test.cc (1)
196-201: Consider documenting the cardinality behavior quirk.The comment mentions "Something weird about attributes hashmap. If cardinality is setup to n, it emits n-1 including overflow." This appears to be intentional behavior based on the implementation in
attributes_hashmap.h, where the overflow entry counts towards the limit.Consider adding a more detailed explanation or linking to documentation about this behavior to help future maintainers understand this is by design.
deps/opentelemetry-cpp/ext/src/http/client/curl/http_operation_curl.cc (2)
360-365: Avoid redundant wait() before get().get() already blocks; remove the extra wait().
- async_data_->result_future.wait(); - last_curl_result_ = async_data_->result_future.get(); + last_curl_result_ = async_data_->result_future.get();
437-441: RAII-guard the callback thread id to ensure reset on early return/throw.Wrap SetThreadId/clear in a small guard to guarantee restoration.
- HttpOperationAccessor::SetThreadId(*async_data_, std::this_thread::get_id()); - callback(*this); - HttpOperationAccessor::SetThreadId(*async_data_, std::thread::id()); + struct CallbackThreadGuard { + HttpOperation::AsyncData &d; + explicit CallbackThreadGuard(HttpOperation::AsyncData &ad) : d(ad) { + HttpOperationAccessor::SetThreadId(d, std::this_thread::get_id()); + } + ~CallbackThreadGuard() { HttpOperationAccessor::SetThreadId(d, std::thread::id()); } + } guard(*async_data_); + callback(*this);deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/common/custom_hash_equality.h (3)
6-11: Add missing include for type traitsRequired for the static type constraints below (and generally useful here).
Apply:
#pragma once #include <cstddef> #include <cstring> +#include <type_traits> #include <string>
81-95: Constrain Fnv1a to unsigned integral types to avoid UB on overflowSigned overflow in C++ is UB. Guard the template to unsigned integrals only.
Apply:
template <typename Ty> inline Ty Fnv1a(const void *buf, size_t len, Ty hval = FnvOffset<Ty, sizeof(Ty)>::value) noexcept { + static_assert(std::is_integral<Ty>::value && std::is_unsigned<Ty>::value, + "Fnv1a requires an unsigned integral hash type"); const unsigned char *bp = reinterpret_cast<const unsigned char *>(buf); const unsigned char *be = bp + len; Ty prime = FnvPrime<Ty, sizeof(Ty)>::value;
144-154: Tighten find_heterogeneous parameter to const&Prevents accidental binding to temporaries and guarantees const-correctness; return type will be const_iterator which is sufficient for lookups.
Apply:
-template <typename MapType> -inline auto find_heterogeneous(MapType &&map, opentelemetry::nostd::string_view key) +template <typename MapType> +inline auto find_heterogeneous(const MapType &map, opentelemetry::nostd::string_view key) { #if defined(_LIBCPP_VERSION) || \ (!defined(OPENTELEMETRY_STL_VERSION) || OPENTELEMETRY_STL_VERSION < 2020) return map.find(std::string(key)); #else // libstdc++ + C++20: heterogeneous lookup works return map.find(key); #endif }deps/opentelemetry-cpp/sdk/test/metrics/attributes_processor_test.cc (4)
34-35: Use heterogeneous lookup helper to avoid libc++ incompatibilitiesDirect unordered_map::find with string_view may fail on libc++ without transparent lookup. Use the provided helper.
Apply:
- EXPECT_FALSE(filter.find(e.first) == filter.end()); + EXPECT_FALSE(find_heterogeneous(filter, e.first) == filter.end());
52-53: Strengthen assertion: verify no attributes pass when allow-list is emptyConfirms behavior, not just filter size.
Apply:
auto filtered_attributes = attributes_processor.process(iterable); EXPECT_EQ(filter.size(), kNumFilterAttributes); + EXPECT_TRUE(filtered_attributes.empty());
71-75: Use heterogeneous lookup helper here as wellEnsures portability across libstdc++/libc++.
Apply:
- EXPECT_TRUE(filter.find(e.first) == filter.end()); + EXPECT_TRUE(find_heterogeneous(filter, e.first) == filter.end());
77-90: Test naming nitThe test name mentions “ExcludeAll” but constructs FilteringExcludeAttributesProcessor with an empty list (exclude none). Consider renaming for clarity.
deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/metrics/view/attributes_processor.h (2)
6-6: Remove unused includeisn’t used in this header; it’s included by custom_hash_equality.h already.
Apply:
-#include <cstring>
26-31: Consider using unordered_set instead of map<string,bool>Value is unused; a set expresses intent better and is marginally smaller. If API stability allows, consider:
Proposed alias (if acceptable):
using FilterAttributeSet = std::unordered_set<std::string, StringViewHash, StringViewEqual>;Then adjust members/lookups accordingly.
deps/opentelemetry-cpp/sdk/src/logs/batch_log_record_processor.cc (1)
383-405: Timeout only applies to exporter->Shutdown; join can block indefinitely—clarify/document semantics and consider a guard
- The provided timeout is not used to bound worker_thread_.join(). If Export blocks, shutdown may exceed the caller’s timeout expectations.
- Also consider guarding against self-join: if destructor could be invoked on the worker thread, join would deadlock. If that situation is impossible by design, add a brief comment; otherwise add a defensive check.
Examples:
- Document that Shutdown’s timeout is applied to exporter shutdown only.
- Optionally add a check before join:
if (std::this_thread::get_id() == worker_thread_.get_id()) { /* unreachable by design; explain */ }deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/trace/samplers/parent.h (1)
43-51: Default args allocate samplers per call; consider avoiding header-level allocations.std::make_shared in default parameters creates new instances on each use and forces header includes. Consider:
- Keep the constructor simple (no defaults) and rely on the factory 1-arg overload to supply defaults; or
- Use shared singletons/static instances in the .cc to reduce allocations and coupling.
Not blocking; existing approach is correct.
deps/opentelemetry-cpp/sdk/test/trace/parent_sampler_test.cc (1)
54-61: Tests cover local/remote and sampled/non-sampled paths—LGTM.Good coverage of all branches and trace_state propagation.
Minor nit: Update the nearby comment at Line 63 to say “root sampler” instead of “delegateSampler”.
Also applies to: 74-79, 81-85, 86-97
deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/trace/samplers/parent_factory.h (1)
26-26: Factory overloads/readability—LGTM.Signatures align with the new ParentBasedSampler API and keep the 1-arg convenience path.
Optional: Document that the 1-arg overload uses AlwaysOn/AlwaysOff for the four per-parent samplers.
Also applies to: 31-36
deps/opentelemetry-cpp/sdk/src/trace/samplers/parent_factory.cc (2)
20-26: Inline the delegating return for brevity.No behavior change.
- std::unique_ptr<Sampler> sampler = ParentBasedSamplerFactory::Create( - root_sampler, std::make_shared<AlwaysOnSampler>(), std::make_shared<AlwaysOffSampler>(), - std::make_shared<AlwaysOnSampler>(), std::make_shared<AlwaysOffSampler>()); - return sampler; + return ParentBasedSamplerFactory::Create(root_sampler, + std::make_shared<AlwaysOnSampler>(), + std::make_shared<AlwaysOffSampler>(), + std::make_shared<AlwaysOnSampler>(), + std::make_shared<AlwaysOffSampler>());
28-39: Prefer make_unique for clarity and exception safety.Construct the derived and return as unique_ptr.
- std::unique_ptr<Sampler> sampler(new ParentBasedSampler( - root_sampler, remote_parent_sampled_sampler, remote_parent_nonsampled_sampler, - local_parent_sampled_sampler, local_parent_nonsampled_sampler)); - return sampler; + return std::make_unique<ParentBasedSampler>(root_sampler, + remote_parent_sampled_sampler, + remote_parent_nonsampled_sampler, + local_parent_sampled_sampler, + local_parent_nonsampled_sampler);deps/opentelemetry-cpp/sdk/src/trace/samplers/parent.cc (1)
23-35: Guard against nullptr root_sampler in ParentBasedSampler ctor
The initializer dereferencesroot_samplerfordescription_, which will crash if null; add anassert(root_sampler != nullptr)or throwstd::invalid_argumentbefore use.
deps/opentelemetry-cpp/sdk/src/trace/samplers/parent.cc:23deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/extension_log_record_processor_builder.h (1)
21-27: Optional: delete copy/move on abstract base to prevent misuse/slicing.Abstract interfaces rarely need copy/move; deleting them clarifies intent.
- ExtensionLogRecordProcessorBuilder(ExtensionLogRecordProcessorBuilder &&) = default; - ExtensionLogRecordProcessorBuilder(const ExtensionLogRecordProcessorBuilder &) = default; - ExtensionLogRecordProcessorBuilder &operator=(ExtensionLogRecordProcessorBuilder &&) = default; - ExtensionLogRecordProcessorBuilder &operator=(const ExtensionLogRecordProcessorBuilder &other) = - default; + ExtensionLogRecordProcessorBuilder(ExtensionLogRecordProcessorBuilder &&) = delete; + ExtensionLogRecordProcessorBuilder(const ExtensionLogRecordProcessorBuilder &) = delete; + ExtensionLogRecordProcessorBuilder &operator=(ExtensionLogRecordProcessorBuilder &&) = delete; + ExtensionLogRecordProcessorBuilder &operator=(const ExtensionLogRecordProcessorBuilder &) = + delete;deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/propagator_configuration.h (1)
22-24: Add doc comment on composite_list semantics
In propagator_configuration.h, document thatcomposite_listis a comma-separated list of propagator names, how it’s parsed versus thecompositevector, and which takes precedence (duplicates are checked globally).deps/opentelemetry-cpp/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_exporter_options.h (1)
35-43: Make the void* constructor explicit
No call sites useOtlpHttpExporterOptions(nullptr|0), so marking this overloadexplicitprevents accidental selection.- /** No defaults. */ - OtlpHttpExporterOptions(void *); + /** No defaults. */ + explicit OtlpHttpExporterOptions(void *);deps/opentelemetry-cpp/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_log_record_exporter_options.h (1)
35-42: Constructor overload looks fine; consider guarding against accidental implicit conversions.Adding the void* sentinel ctor and defaulted copy/move ops is OK. To avoid accidental implicit construction from nullptr or 0 in user code, consider making the sentinel constructor explicit.
- /** No defaults. */ - OtlpHttpLogRecordExporterOptions(void *); + /** No defaults. */ + explicit OtlpHttpLogRecordExporterOptions(void *);deps/opentelemetry-cpp/api/include/opentelemetry/common/string_util.h (1)
6-7: Include is correct; consider guarding isspace() against signed char UB.With <ctype.h> included, cast to unsigned char before calling isspace to avoid UB for negative chars.
Example change (outside this hunk):
while (left <= right && isspace(static_cast<unsigned char>(str[left]))) { ... } while (left <= right && isspace(static_cast<unsigned char>(str[right]))) { ... }deps/opentelemetry-cpp/exporters/prometheus/src/exporter_options.cc (1)
80-81: Mark PrometheusExporterOptions(void) explicit.*The void* sentinel ctor in deps/opentelemetry-cpp/exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_options.h is not marked explicit; add explicit to avoid implicit nullptr/0→PrometheusExporterOptions conversions. Call sites set url (deps/.../prometheus/src/prometheus_pull_builder.cc uses options(nullptr) then assigns options.url; tests also assign url), so behavior is safe today but making the ctor explicit prevents accidental conversions.
deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/open_census_metric_producer_configuration.h (1)
23-26: Align Accept signature with possible noexcept on baseIf MetricProducerConfiguration::Accept is noexcept, the override should be too. Minor polish, avoids exception-spec mismatches.
Proposed tweak:
- void Accept(MetricProducerConfigurationVisitor *visitor) const override + void Accept(MetricProducerConfigurationVisitor *visitor) const noexcept overridePlease verify the base signature in metric_producer_configuration.h to confirm noexcept.
deps/opentelemetry-cpp/api/test/core/version_test.cc (2)
13-21: Avoid fixed-size buffer; build expected via std::stringPrevents future truncation if version components grow; simplifies logic.
Apply:
- char expected[10]; - snprintf(expected, sizeof(expected), "%d.%d.%d", OPENTELEMETRY_VERSION_MAJOR, - OPENTELEMETRY_VERSION_MINOR, OPENTELEMETRY_VERSION_PATCH); - - std::string actual = OPENTELEMETRY_VERSION; - /* OPENTELEMETRY_VERSION may contain a -dev suffix */ - std::string prefix = actual.substr(0, strlen(expected)); - EXPECT_EQ(prefix, expected); + std::string expected = std::to_string(OPENTELEMETRY_VERSION_MAJOR) + "." + + std::to_string(OPENTELEMETRY_VERSION_MINOR) + "." + + std::to_string(OPENTELEMETRY_VERSION_PATCH); + + std::string actual = OPENTELEMETRY_VERSION; + /* OPENTELEMETRY_VERSION may contain a -dev suffix */ + EXPECT_EQ(0, actual.compare(0, expected.size(), expected));
6-6: Drop string.h if refactor is appliedIf you move to std::string compare above, <string.h> is no longer needed.
-#include <string.h>deps/opentelemetry-cpp/exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_options.h (1)
21-26: Safer sentinel constructor signatureUsing a void* overload as a sentinel can accept any pointer implicitly. Consider either:
- Make it explicit, or
- Prefer PrometheusExporterOptions(std::nullptr_t) to only allow nullptr.
This reduces accidental matches while preserving the intent (“no defaults” path).
If this mirrors upstream exactly, keeping it as-is may be preferable for future vendor syncs. Please confirm.
deps/opentelemetry-cpp/ext/test/http/CMakeLists.txt (1)
7-10: Restore a fallback when CURL::libcurl imported target is unavailableDirectly linking CURL::libcurl assumes the imported target exists. Some environments use CURL_INCLUDE_DIRS/CURL_LIBRARIES without the imported target. Add a small fallback to avoid breaking builds on older toolchains.
Apply this diff:
- target_link_libraries( - ${FILENAME} - PRIVATE ${GMOCK_LIB} ${GTEST_BOTH_LIBRARIES} opentelemetry_http_client_curl - opentelemetry_common CURL::libcurl) + target_link_libraries( + ${FILENAME} + PRIVATE ${GMOCK_LIB} ${GTEST_BOTH_LIBRARIES} opentelemetry_http_client_curl + opentelemetry_common) + if (TARGET CURL::libcurl) + target_link_libraries(${FILENAME} PRIVATE CURL::libcurl) + else() + find_package(CURL REQUIRED) + target_include_directories(${FILENAME} PRIVATE ${CURL_INCLUDE_DIRS}) + target_link_libraries(${FILENAME} PRIVATE ${CURL_LIBRARIES}) + endif()deps/opentelemetry-cpp/exporters/ostream/src/console_log_record_builder.cc (1)
27-32: Missing null check on configuration.
RegistryinvokesBuild(nullptr)when no console exporter config exists. Even though this implementation ignores the model, adding a check avoids accidental dereference if parameter usage changes later.Return the exporter immediately when
modelis null:std::unique_ptr<opentelemetry::sdk::logs::LogRecordExporter> ConsoleLogRecordBuilder::Build( - const opentelemetry::sdk::configuration::ConsoleLogRecordExporterConfiguration * /* model */) + const opentelemetry::sdk::configuration::ConsoleLogRecordExporterConfiguration * model) const { + if (model == nullptr) + { + return OStreamLogRecordExporterFactory::Create(); + } return OStreamLogRecordExporterFactory::Create(); }deps/opentelemetry-cpp/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_grpc_span_builder.h (1)
24-28: Expose configuration in the builder interfaceWe should mirror other builder headers by acknowledging the configuration parameter in the signature (even if unused internally) so downstream overrides can propagate options cleanly. Rename the argument from
modelto something meaningful or document why it can be null to avoid ambiguity.deps/opentelemetry-cpp/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_log_record_builder.h (1)
24-28: Consistently surface the configuration parameterPlease mirror our other headers by giving the
modelargument a descriptive name or documenting nullability so implementers know how to convert it into exporter options.deps/opentelemetry-cpp/exporters/zipkin/include/opentelemetry/exporters/zipkin/zipkin_builder.h (1)
24-28: Document model usageCould we add brief documentation (or a TODO) clarifying how the
ZipkinSpanExporterConfigurationis expected to feed into exporter options? It helps downstream implementers when scanning the header.deps/opentelemetry-cpp/exporters/otlp/src/otlp_builder_utils.cc (1)
51-51: Use empty() instead of size() > 0Minor readability/perf improvement.
Apply this diff:
- if (headers_list.size() > 0) + if (!headers_list.empty())deps/opentelemetry-cpp/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_builder_utils.h (1)
28-31: Consider avoiding unnecessary string copies.If build settings allow, prefer string_view for headers_list to avoid copying large header strings during conversion.
deps/opentelemetry-cpp/exporters/otlp/src/otlp_file_log_record_builder.cc (1)
22-26: Null-check registry before registration.Guard against accidental nullptr to avoid UB.
Apply this diff:
void OtlpFileLogRecordBuilder::Register(opentelemetry::sdk::configuration::Registry *registry) { - auto builder = std::make_unique<OtlpFileLogRecordBuilder>(); - registry->SetOtlpFileLogRecordBuilder(std::move(builder)); + if (!registry) + { + return; + } + auto builder = std::make_unique<OtlpFileLogRecordBuilder>(); + registry->SetOtlpFileLogRecordBuilder(std::move(builder)); }deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/extension_sampler_builder.h (1)
18-30: Export the interface to ensure vtable visibility across DLL boundaries.SDK public interface classes should be exported to avoid Windows/shared-lib issues.
Apply this diff:
-class ExtensionSamplerBuilder +class OPENTELEMETRY_EXPORT ExtensionSamplerBuilder {deps/opentelemetry-cpp/exporters/zipkin/include/opentelemetry/exporters/zipkin/zipkin_exporter_options.h (2)
17-27: Drop const on by-value return.Returning
const std::stringis unnecessary and can inhibit moves.Apply this diff:
-inline const std::string GetDefaultZipkinEndpoint() +inline std::string GetDefaultZipkinEndpoint()
40-45: Prefer a tag type overvoid*for “no defaults” constructor.A distinct tag (e.g., struct NoDefaults{}) avoids accidental matches and clarifies intent.
deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/extension_span_processor_builder.h (1)
28-31: Prefer const reference over raw pointer for model parameter (or document non-null).Using a raw pointer invites accidental nulls. Consider a const reference to express required input, or add a clear nullability contract.
Apply this diff if you opt for a const reference:
- virtual std::unique_ptr<opentelemetry::sdk::trace::SpanProcessor> Build( - const opentelemetry::sdk::configuration::ExtensionSpanProcessorConfiguration *model) + virtual std::unique_ptr<opentelemetry::sdk::trace::SpanProcessor> Build( + const opentelemetry::sdk::configuration::ExtensionSpanProcessorConfiguration &model) const = 0;deps/opentelemetry-cpp/exporters/prometheus/src/prometheus_pull_builder.cc (2)
23-27: Guard against null registry in Register.A simple check prevents UB if called with nullptr.
void PrometheusPullBuilder::Register(opentelemetry::sdk::configuration::Registry *registry) { - auto builder = std::make_unique<PrometheusPullBuilder>(); - registry->SetPrometheusPullMetricExporterBuilder(std::move(builder)); + if (!registry) + { + return; + } + auto builder = std::make_unique<PrometheusPullBuilder>(); + registry->SetPrometheusPullMetricExporterBuilder(std::move(builder)); }
29-37: Validate model and port; handle empty host.Minor hardening: check model for null, validate port range, and fall back if host is empty.
std::unique_ptr<opentelemetry::sdk::metrics::MetricReader> PrometheusPullBuilder::Build( const opentelemetry::sdk::configuration::PrometheusPullMetricExporterConfiguration *model) const { - opentelemetry::exporter::metrics::PrometheusExporterOptions options(nullptr); + if (model == nullptr) + { + return nullptr; + } + opentelemetry::exporter::metrics::PrometheusExporterOptions options(nullptr); - std::string url(model->host); - url.append(":"); - url.append(std::to_string(model->port)); + // Default host if empty and validate port + const std::string host = model->host.empty() ? std::string("localhost") : model->host; + const uint16_t port = static_cast<uint16_t>(model->port); // truncates >65535, consider clamping + std::string url = host + ":" + std::to_string(port);Note: If truncation is undesirable, clamp or return nullptr when port is out of [1, 65535].
deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/zipkin_span_exporter_builder.h (1)
28-30: Use const reference for configuration parameter (or document non-null).Same pattern as other builders; using const& avoids nullable API surface and clarifies ownership.
- virtual std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> Build( - const opentelemetry::sdk::configuration::ZipkinSpanExporterConfiguration *model) const = 0; + virtual std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> Build( + const opentelemetry::sdk::configuration::ZipkinSpanExporterConfiguration &model) const = 0;deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/console_span_exporter_builder.h (1)
28-30: Align parameter with const reference or add nullability docs.Prefer const& to avoid nullable pointer usage at call sites.
- virtual std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> Build( - const opentelemetry::sdk::configuration::ConsoleSpanExporterConfiguration *model) const = 0; + virtual std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> Build( + const opentelemetry::sdk::configuration::ConsoleSpanExporterConfiguration &model) const = 0;deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/otlp_http_log_record_exporter_builder.h (1)
29-31: Use const reference or assert non-null.Same reasoning across builder interfaces.
- virtual std::unique_ptr<opentelemetry::sdk::logs::LogRecordExporter> Build( - const opentelemetry::sdk::configuration::OtlpHttpLogRecordExporterConfiguration *model) + virtual std::unique_ptr<opentelemetry::sdk::logs::LogRecordExporter> Build( + const opentelemetry::sdk::configuration::OtlpHttpLogRecordExporterConfiguration &model) const = 0;deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/otlp_file_span_exporter_builder.h (1)
28-30: Const reference for model parameter.Consistent, safer API surface; avoids nullable input.
- virtual std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> Build( - const opentelemetry::sdk::configuration::OtlpFileSpanExporterConfiguration *model) const = 0; + virtual std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> Build( + const opentelemetry::sdk::configuration::OtlpFileSpanExporterConfiguration &model) const = 0;deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/extension_pull_metric_exporter_builder.h (1)
29-31: Prefer const reference to avoid nullable configuration pointer.Keeps the interface non-null by construction.
- virtual std::unique_ptr<opentelemetry::sdk::metrics::MetricReader> Build( - const opentelemetry::sdk::configuration::ExtensionPullMetricExporterConfiguration *model) + virtual std::unique_ptr<opentelemetry::sdk::metrics::MetricReader> Build( + const opentelemetry::sdk::configuration::ExtensionPullMetricExporterConfiguration &model) const = 0;deps/opentelemetry-cpp/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_grpc_metric_exporter_options.h (1)
29-30: Confirm intent behind the void “no defaults” ctor*Using a void* tag-ctor is easy to call unintentionally (e.g., with 0/null pointer constants). If this mirrors upstream API, fine; otherwise prefer a strongly-typed tag (or std::nullptr_t) to avoid accidental matches.
Would you confirm all internal call sites use nullptr explicitly (never 0 or arbitrary pointers)?
deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/otlp_file_log_record_exporter_builder.h (2)
18-20: Consider symbol export if this crosses shared-lib boundaries (Windows)If this type is consumed across DLL boundaries, it may need OPENTELEMETRY_EXPORT on the class (or on the virtual functions) for proper symbol visibility on Windows.
29-31: Pointer parameter implies nullable; consider clarifying non-null preconditionIf model is required non-null, a reference (or an assertion) would better express that. If nullable, document behavior on nullptr.
deps/opentelemetry-cpp/exporters/otlp/src/otlp_grpc_push_metric_builder.cc (1)
25-29: Add a defensive null check (optional) or document preconditionRegister dereferences registry without checking. If callers could pass nullptr, guard or document non-null requirement.
void OtlpGrpcPushMetricBuilder::Register(opentelemetry::sdk::configuration::Registry *registry) { - auto builder = std::make_unique<OtlpGrpcPushMetricBuilder>(); - registry->SetOtlpGrpcPushMetricExporterBuilder(std::move(builder)); + if (!registry) return; + auto builder = std::make_unique<OtlpGrpcPushMetricBuilder>(); + registry->SetOtlpGrpcPushMetricExporterBuilder(std::move(builder)); }deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/otlp_grpc_log_record_exporter_builder.h (1)
18-32: Consider marking Build as [[nodiscard]] and documenting nullability of modelReturning a unique_ptr from Build can be accidentally ignored. Also, clarify whether model can be null (or switch to reference if not intended).
Apply this diff to add [[nodiscard]]:
- virtual std::unique_ptr<opentelemetry::sdk::logs::LogRecordExporter> Build( + [[nodiscard]] virtual std::unique_ptr<opentelemetry::sdk::logs::LogRecordExporter> Build( const opentelemetry::sdk::configuration::OtlpGrpcLogRecordExporterConfiguration *model) const = 0;deps/opentelemetry-cpp/sdk/src/configuration/configured_sdk.cc (1)
33-57: Handle null registry or log an explicit messageCreate() ignores a null registry. Consider validating it and logging to aid diagnostics.
- if (model) + if (model) { try { - SdkBuilder builder(std::move(registry)); + SdkBuilder builder(std::move(registry));(Just add a check/log before builder if registry is null.)
deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/extension_push_metric_exporter_builder.h (1)
18-32: Add [[nodiscard]] to Build and document nullabilitySame note as other builders: prevent ignored results and clarify if model can be null.
Apply this diff:
- virtual std::unique_ptr<opentelemetry::sdk::metrics::PushMetricExporter> Build( + [[nodiscard]] virtual std::unique_ptr<opentelemetry::sdk::metrics::PushMetricExporter> Build( const opentelemetry::sdk::configuration::ExtensionPushMetricExporterConfiguration *model) const = 0;deps/opentelemetry-cpp/exporters/otlp/src/otlp_http_push_metric_builder.cc (1)
37-55: Verify mapping completeness and security flags
- ssl_insecure_skip_verify is hard-coded to false. If the config model exposes an “insecure” or “skip verify” flag, consider honoring it (default secure).
- Only certificate/key file paths are mapped. If the model supports inline PEM strings, map those too.
- options constructed with nullptr: confirm the intended ctor overload.
If needed, extend the mapping to include:
- ssl_ca_cert_string / ssl_client_key_string / ssl_client_cert_string
- retry policy, user agent, and concurrency limits (if present in model)
deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/extension_log_record_exporter_builder.h (1)
29-31: Clarify ownership/nullability of model (prefer reference over pointer)Taking a raw pointer suggests nullable input. If null is not expected, prefer a const reference to make the contract explicit and avoid null checks.
Apply this diff:
- virtual std::unique_ptr<opentelemetry::sdk::logs::LogRecordExporter> Build( - const opentelemetry::sdk::configuration::ExtensionLogRecordExporterConfiguration *model) + virtual std::unique_ptr<opentelemetry::sdk::logs::LogRecordExporter> Build( + const opentelemetry::sdk::configuration::ExtensionLogRecordExporterConfiguration &model) const = 0;deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/configured_sdk.h (1)
51-55: Public mutable members risk post-install mutationExposing providers/resources as public members allows accidental mutation after Install(). Consider making them private with accessors to preserve invariants.
deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/otlp_http_push_metric_exporter_builder.h (2)
9-9: Unused include can be dropped
push_metric_exporter_configuration.hisn’t referenced in this header. Removing it reduces compile time and coupling.Apply this diff:
-#include "opentelemetry/sdk/configuration/push_metric_exporter_configuration.h"
30-32: Prefer const reference for non-null modelAs above, use a const reference to make non-null contract explicit (or document/null-check if null is allowed).
Apply this diff:
- virtual std::unique_ptr<opentelemetry::sdk::metrics::PushMetricExporter> Build( - const opentelemetry::sdk::configuration::OtlpHttpPushMetricExporterConfiguration *model) + virtual std::unique_ptr<opentelemetry::sdk::metrics::PushMetricExporter> Build( + const opentelemetry::sdk::configuration::OtlpHttpPushMetricExporterConfiguration &model) const = 0;deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/console_push_metric_exporter_builder.h (1)
29-31: Make model non-nullable (const reference)Align with other builders and avoid raw pointer unless null is meaningful.
Apply this diff:
- virtual std::unique_ptr<opentelemetry::sdk::metrics::PushMetricExporter> Build( - const opentelemetry::sdk::configuration::ConsolePushMetricExporterConfiguration *model) + virtual std::unique_ptr<opentelemetry::sdk::metrics::PushMetricExporter> Build( + const opentelemetry::sdk::configuration::ConsolePushMetricExporterConfiguration &model) const = 0;deps/opentelemetry-cpp/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_grpc_log_record_exporter_options.h (1)
29-29: Make the tag constructor explicit (and noexcept)A
void*tag constructor is easily invoked implicitly (e.g., withnullptr). Mark it explicit (and consider noexcept) to prevent accidental use.Apply this diff:
- OtlpGrpcLogRecordExporterOptions(void *); + explicit OtlpGrpcLogRecordExporterOptions(void *) noexcept;deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/metric_producer_configuration_visitor.h (1)
28-30: Prefer references over pointers in visitor methodsIf these models are required, using const references expresses non-null intent and avoids pointer null checks across implementations. Keep as-is if matching existing visitor APIs elsewhere.
deps/opentelemetry-cpp/sdk/src/configuration/configuration_parser.cc (1)
1112-1146: Propagator composite parsing: minor clarity improvements
- name is declared outside the loop; define it inside the element scope to avoid reuse across iterations.
- num_child can be size_t for consistency with counts.
Example tweak:
- std::string name; - int num_child = 0; + size_t num_child = 0; if (child) { for (auto it = child->begin(); it != child->end(); ++it) { std::unique_ptr<DocumentNode> element(*it); num_child++; - int count = 0; - for (auto it2 = element->begin_properties(); it2 != element->end_properties(); ++it2) + int count = 0; + std::string name; + for (auto it2 = element->begin_properties(); it2 != element->end_properties(); ++it2) { name = it2.Name(); count++; }deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/console_log_record_exporter_builder.h (1)
29-31: Consider marking Build as nodiscardReturning a heap-allocated exporter is easy to accidentally ignore. [[nodiscard]] (if allowed in this codebase) would help catch that at compile time.
deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/sdk_builder.h (3)
69-75: Consider making the constructor explicit.Prevents unintended implicit conversions from shared_ptr.
- SdkBuilder(std::shared_ptr<Registry> registry) : registry_(std::move(registry)) {} + explicit SdkBuilder(std::shared_ptr<Registry> registry) : registry_(std::move(registry)) {}
136-142: Name-based propagator creation depends on Registry defaults; document supported names.Please document the supported names ("tracecontext", "baggage", "b3", "b3multi", "jaeger") to avoid config ambiguity.
199-206: Attributes processor not implemented yet.CreateAttributesProcessor() returns nullptr (see .cc). Consider clarifying this in the header to set expectations.
deps/opentelemetry-cpp/api/include/opentelemetry/semconv/incubating/gen_ai_attributes.h (1)
613-624: Likewise, deprecate az.ai.openai in favor of azure.ai.openai.Keep one canonical token to avoid split usage.
-/** - Azure OpenAI - */ -static constexpr const char *kAzAiOpenai = "az.ai.openai"; +/** + Azure OpenAI + @deprecated + {"note": "Replaced by @code azure.ai.openai @endcode.", "reason": "renamed", "renamed_to": "azure.ai.openai"} + */ +OPENTELEMETRY_DEPRECATED static constexpr const char *kAzAiOpenai = "az.ai.openai";deps/opentelemetry-cpp/sdk/src/configuration/registry.cc (2)
90-106: Registry access is not thread-safe.If used after SDK install, concurrent Get/Set could race. If intentional, consider documenting "initialize before multithreaded use" or guard Set* with a mutex.
Would you like a minimal patch adding a std::mutex around Set* methods?
101-106: Use map::insert_or_assign for brevity.Removes erase+insert pairs and conveys intent.
- propagator_builders_.erase(name); - propagator_builders_.insert({name, std::move(builder)}); + propagator_builders_.insert_or_assign(name, std::move(builder));Apply similarly to other Set* maps in this file.
deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/registry.h (1)
249-279: Consider documenting thread-safety.Public API exposes Get*/Set* that mutate internal maps; clarifying single-threaded initialization expectations would help users avoid races.
deps/opentelemetry-cpp/sdk/src/configuration/sdk_builder.cc (2)
1064-1076: Simplify and short-circuit duplicate check.Avoids unnecessary scans.
-static bool IsDuplicate(const std::vector<std::string> &propagator_seen, const std::string &name) -{ - bool duplicate = false; - for (const auto &seen : propagator_seen) - { - if (name == seen) - { - duplicate = true; - } - } - return duplicate; -} +static bool IsDuplicate(const std::vector<std::string> &propagator_seen, const std::string &name) +{ + for (const auto &seen : propagator_seen) + { + if (name == seen) return true; + } + return false; +}
1106-1138: Trim whitespace when parsing composite_list.Tokens with spaces ("b3, tracecontext") will be treated as different names. Trim to reduce config surprises.
- while ((end_pos = str_list.find(separator, start_pos)) != std::string::npos) + while ((end_pos = str_list.find(separator, start_pos)) != std::string::npos) { - name = str_list.substr(start_pos, end_pos - start_pos); + name = str_list.substr(start_pos, end_pos - start_pos); + // trim + auto first = name.find_first_not_of(" \t"); + auto last = name.find_last_not_of(" \t"); + name = (first == std::string::npos) ? std::string{} : name.substr(first, last - first + 1); @@ - name = str_list.substr(start_pos); + name = str_list.substr(start_pos); + // trim + auto first = name.find_first_not_of(" \t"); + auto last = name.find_last_not_of(" \t"); + name = (first == std::string::npos) ? std::string{} : name.substr(first, last - first + 1);deps/opentelemetry-cpp/sdk/test/metrics/histogram_aggregation_benchmark.cc (1)
119-121: API alignment LGTM; consider make_uniqueThe ctor call matches the new View signature. Optionally simplify with make_unique.
- std::unique_ptr<View> histogram_view{ - new View("base2_expohisto", "description", AggregationType::kBase2ExponentialHistogram, - std::make_shared<Base2ExponentialHistogramAggregationConfig>(config))}; + auto histogram_view = std::make_unique<View>( + "base2_expohisto", "description", AggregationType::kBase2ExponentialHistogram, + std::make_shared<Base2ExponentialHistogramAggregationConfig>(config));deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/logs/batch_log_record_processor_options.h (1)
19-20: Docstring alignment LGTM; minor nearby terminology nitComment now correctly refers to LogRecordProcessor. While here, consider fixing the next block’s “spans are dropped” to “logs/log records are dropped” to avoid copy-paste confusion.
deps/opentelemetry-cpp/sdk/src/trace/batch_span_processor.cc (1)
373-395: Call exporter->Shutdown outside the shutdown mutex to avoid potential deadlocksHolding shutdown_m while invoking exporter->Shutdown can deadlock or block other Shutdown callers unnecessarily if the exporter is slow or re-entrant. Compute the “should call exporter” decision under the lock, release it, then invoke exporter->Shutdown.
Apply this diff:
bool BatchSpanProcessor::InternalShutdown(std::chrono::microseconds timeout) noexcept { - auto start_time = std::chrono::system_clock::now(); - std::lock_guard<std::mutex> shutdown_guard{synchronization_data_->shutdown_m}; - bool already_shutdown = synchronization_data_->is_shutdown.exchange(true); - - if (worker_thread_.joinable()) - { - synchronization_data_->is_force_wakeup_background_worker.store(true, std::memory_order_release); - synchronization_data_->cv.notify_all(); - worker_thread_.join(); - } - - GetWaitAdjustedTime(timeout, start_time); - // Should only shutdown exporter ONCE. - if (!already_shutdown && exporter_ != nullptr) - { - return exporter_->Shutdown(timeout); - } - - return true; + auto start_time = std::chrono::system_clock::now(); + bool call_exporter_shutdown = false; + { + std::lock_guard<std::mutex> shutdown_guard{synchronization_data_->shutdown_m}; + bool already_shutdown = synchronization_data_->is_shutdown.exchange(true); + + if (worker_thread_.joinable()) + { + synchronization_data_->is_force_wakeup_background_worker.store(true, std::memory_order_release); + synchronization_data_->cv.notify_all(); + worker_thread_.join(); + } + // Decide under lock; perform the call after releasing it. + call_exporter_shutdown = (!already_shutdown && exporter_ != nullptr); + } + + GetWaitAdjustedTime(timeout, start_time); + if (call_exporter_shutdown) + { + return exporter_->Shutdown(timeout); + } + return true; }deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/logs/batch_log_record_processor.h (1)
111-117: Approve code changes; polish destructor documentation
The destructor comment currently states it invokes Shutdown(); revise it to clarify it calls InternalShutdown (no behavior change).deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/extension_span_exporter_builder.h (3)
28-29: Enforce non-null configuration (avoid nullable pointer in API).Build takes a raw pointer and implementations dereference it; prefer const& or document/assert non-null to prevent NPEs.
Would you consider switching to:
- virtual std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> Build( - const opentelemetry::sdk::configuration::ExtensionSpanExporterConfiguration *model) const = 0; + virtual std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> Build( + const opentelemetry::sdk::configuration::ExtensionSpanExporterConfiguration &model) const = 0;If changing the signature is not feasible due to API stability, please document non-null and ensure all callers pass a valid object.
21-26: Abstract builders generally shouldn’t be copyable.Defaulted copy/move on an abstract base invites accidental slicing; consider deleting copy/assign.
- ExtensionSpanExporterBuilder(ExtensionSpanExporterBuilder &&) = default; - ExtensionSpanExporterBuilder(const ExtensionSpanExporterBuilder &) = default; - ExtensionSpanExporterBuilder &operator=(ExtensionSpanExporterBuilder &&) = default; - ExtensionSpanExporterBuilder &operator=(const ExtensionSpanExporterBuilder &other) = default; + ExtensionSpanExporterBuilder(ExtensionSpanExporterBuilder &&) = delete; + ExtensionSpanExporterBuilder(const ExtensionSpanExporterBuilder &) = delete; + ExtensionSpanExporterBuilder &operator=(ExtensionSpanExporterBuilder &&) = delete; + ExtensionSpanExporterBuilder &operator=(const ExtensionSpanExporterBuilder &other) = delete;
18-30: Consider symbol export for shared-library ABI.If this interface is consumed across DLL/DSO boundaries, mark it with export visibility (e.g., OPENTELEMETRY_EXPORT) to avoid vtable visibility issues on Windows.
Please confirm how similar builder interfaces are annotated elsewhere in the SDK for consistency.
deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/otlp_http_span_exporter_builder.h (2)
28-30: Non-null configuration contract.Same as other builders: prefer const& over a raw pointer or assert/document non-null at call sites to avoid null dereference.
- virtual std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> Build( - const opentelemetry::sdk::configuration::OtlpHttpSpanExporterConfiguration *model) const = 0; + virtual std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> Build( + const opentelemetry::sdk::configuration::OtlpHttpSpanExporterConfiguration &model) const = 0;
18-30: Export visibility for public ABI.If this type is part of the shared public surface, consider adding an export macro to the class.
deps/opentelemetry-cpp/exporters/zipkin/src/zipkin_builder.cc (1)
4-11: Add assert include for defensive checks.You’ll add asserts below; include .
#include <memory> #include <string> #include <utility> +#include <cassert>deps/opentelemetry-cpp/exporters/otlp/src/otlp_http_span_builder.cc (1)
4-8: Include for added asserts.#include <chrono> #include <memory> #include <string> #include <utility> +#include <cassert>deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/extension_metric_producer_configuration.h (1)
23-26: Defensive null check for visitor.Accept dereferences visitor without validation; add assert to prevent UB if a null is passed inadvertently.
void Accept(MetricProducerConfigurationVisitor *visitor) const override { - visitor->VisitExtension(this); + assert(visitor != nullptr); + if (visitor) + { + visitor->VisitExtension(this); + } }Additionally, include near the other includes if not already present:
#include <memory> #include <string> +#include <cassert>
| static constexpr const char *kAzAiInference = "az.ai.inference"; | ||
|
|
||
| /** | ||
| Azure OpenAI | ||
| */ | ||
| static constexpr const char *kAzureAiOpenai = "azure.ai.openai"; | ||
| static constexpr const char *kAzAiOpenai = "az.ai.openai"; | ||
|
|
There was a problem hiding this comment.
Mark short “az.*” aliases as deprecated to avoid conflicting synonyms.
Both az.ai.inference (kAzAiInference) and azure.ai.inference (kAzureAiInference) are defined but only kXai is flagged deprecated among short forms. To reduce ambiguity and nudge users to the canonical names, deprecate kAzAiInference.
-/**
- Azure AI Inference
- */
-static constexpr const char *kAzAiInference = "az.ai.inference";
+/**
+ Azure AI Inference
+ @deprecated
+ {"note": "Replaced by @code azure.ai.inference @endcode.", "reason": "renamed", "renamed_to": "azure.ai.inference"}
+ */
+OPENTELEMETRY_DEPRECATED static constexpr const char *kAzAiInference = "az.ai.inference";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static constexpr const char *kAzAiInference = "az.ai.inference"; | |
| /** | |
| Azure OpenAI | |
| */ | |
| static constexpr const char *kAzureAiOpenai = "azure.ai.openai"; | |
| static constexpr const char *kAzAiOpenai = "az.ai.openai"; | |
| /** | |
| Azure AI Inference | |
| @deprecated | |
| {"note": "Replaced by @code azure.ai.inference @endcode.", "reason": "renamed", "renamed_to": "azure.ai.inference"} | |
| */ | |
| OPENTELEMETRY_DEPRECATED static constexpr const char *kAzAiInference = "az.ai.inference"; | |
| /** | |
| Azure OpenAI | |
| */ | |
| static constexpr const char *kAzAiOpenai = "az.ai.openai"; |
🤖 Prompt for AI Agents
deps/opentelemetry-cpp/api/include/opentelemetry/semconv/incubating/gen_ai_attributes.h
around lines 608-614: the short alias kAzAiInference should be marked deprecated
to discourage its use in favor of the canonical name; change the declaration to
apply the C++ deprecated attribute (e.g. [[deprecated("use kAzureAiInference")]]
or project-deprecation-macro) on the static constexpr const char *kAzAiInference
and update the adjacent comment to note deprecation so consumers receive
compile-time warnings and guidance to use kAzureAiInference instead.
| static constexpr const char *kMetricHwHostHeatingMargin = "hw.host.heating_margin"; | ||
| static constexpr const char *descrMetricHwHostHeatingMargin = | ||
| "By how many degrees Celsius the temperature of the physical host can be increased, before reaching a warning threshold on one of the internal sensors. | ||
| "; | ||
| static constexpr const char *unitMetricHwHostHeatingMargin = "Cel"; | ||
|
|
||
| #if OPENTELEMETRY_ABI_VERSION_NO >= 2 | ||
|
|
||
| static inline nostd::unique_ptr<metrics::Gauge<int64_t>> CreateSyncInt64MetricHwHostHeatingMargin( | ||
| metrics::Meter *meter) | ||
| { |
There was a problem hiding this comment.
Fix unterminated string literal in descrMetricHwHostHeatingMargin.
The descriptor literal currently spans a raw newline, which makes the header fail to compile (unterminated string). Please fold the sentence into standard concatenated literals.
-static constexpr const char *descrMetricHwHostHeatingMargin =
- "By how many degrees Celsius the temperature of the physical host can be increased, before reaching a warning threshold on one of the internal sensors.
- ";
+static constexpr const char *descrMetricHwHostHeatingMargin =
+ "By how many degrees Celsius the temperature of the physical host can be increased, before "
+ "reaching a warning threshold on one of the internal sensors.";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static constexpr const char *kMetricHwHostHeatingMargin = "hw.host.heating_margin"; | |
| static constexpr const char *descrMetricHwHostHeatingMargin = | |
| "By how many degrees Celsius the temperature of the physical host can be increased, before reaching a warning threshold on one of the internal sensors. | |
| "; | |
| static constexpr const char *unitMetricHwHostHeatingMargin = "Cel"; | |
| #if OPENTELEMETRY_ABI_VERSION_NO >= 2 | |
| static inline nostd::unique_ptr<metrics::Gauge<int64_t>> CreateSyncInt64MetricHwHostHeatingMargin( | |
| metrics::Meter *meter) | |
| { | |
| static constexpr const char *kMetricHwHostHeatingMargin = "hw.host.heating_margin"; | |
| static constexpr const char *descrMetricHwHostHeatingMargin = | |
| "By how many degrees Celsius the temperature of the physical host can be increased, before " | |
| "reaching a warning threshold on one of the internal sensors."; | |
| static constexpr const char *unitMetricHwHostHeatingMargin = "Cel"; | |
| #if OPENTELEMETRY_ABI_VERSION_NO >= 2 | |
| static inline nostd::unique_ptr<metrics::Gauge<int64_t>> CreateSyncInt64MetricHwHostHeatingMargin( | |
| metrics::Meter *meter) | |
| { | |
| // … | |
| } |
🤖 Prompt for AI Agents
In
deps/opentelemetry-cpp/api/include/opentelemetry/semconv/incubating/hw_metrics.h
around lines 693 to 703, the descrMetricHwHostHeatingMargin string is split
across a raw newline causing an unterminated string literal; replace the broken
literal with either a single-line quoted string or adjacent C++ string literals
concatenated (e.g. "By how many degrees Celsius the temperature of the physical
host can be increased, before reaching a warning threshold on one of the
internal sensors.") so the header compiles cleanly and preserves the sentence
text (you can also include an explicit "\n" if a newline is desired).
| static constexpr const char *kMetricHwHostPower = "hw.host.power"; | ||
| static constexpr const char *descrMetricHwHostPower = | ||
| "Instantaneous power consumed by the entire physical host in Watts (`hw.host.energy` is preferred). | ||
| "; | ||
| static constexpr const char *unitMetricHwHostPower = "W"; | ||
|
|
||
| #if OPENTELEMETRY_ABI_VERSION_NO >= 2 | ||
|
|
||
| static inline nostd::unique_ptr<metrics::Gauge<int64_t>> CreateSyncInt64MetricHwHostPower( | ||
| metrics::Meter *meter) | ||
| { |
There was a problem hiding this comment.
Close the multiline literal for descrMetricHwHostPower.
Same issue here: the newline inside the quoted literal yields an unterminated-string compilation error. Please collapse it into standard concatenated literals.
-static constexpr const char *descrMetricHwHostPower =
- "Instantaneous power consumed by the entire physical host in Watts (`hw.host.energy` is preferred).
- ";
+static constexpr const char *descrMetricHwHostPower =
+ "Instantaneous power consumed by the entire physical host in Watts (`hw.host.energy` is "
+ "preferred).";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static constexpr const char *kMetricHwHostPower = "hw.host.power"; | |
| static constexpr const char *descrMetricHwHostPower = | |
| "Instantaneous power consumed by the entire physical host in Watts (`hw.host.energy` is preferred). | |
| "; | |
| static constexpr const char *unitMetricHwHostPower = "W"; | |
| #if OPENTELEMETRY_ABI_VERSION_NO >= 2 | |
| static inline nostd::unique_ptr<metrics::Gauge<int64_t>> CreateSyncInt64MetricHwHostPower( | |
| metrics::Meter *meter) | |
| { | |
| static constexpr const char *kMetricHwHostPower = "hw.host.power"; | |
| static constexpr const char *descrMetricHwHostPower = | |
| "Instantaneous power consumed by the entire physical host in Watts (`hw.host.energy` is " | |
| "preferred)."; | |
| static constexpr const char *unitMetricHwHostPower = "W"; | |
| #if OPENTELEMETRY_ABI_VERSION_NO >= 2 | |
| static inline nostd::unique_ptr<metrics::Gauge<int64_t>> CreateSyncInt64MetricHwHostPower( | |
| metrics::Meter *meter) | |
| { |
🤖 Prompt for AI Agents
In
deps/opentelemetry-cpp/api/include/opentelemetry/semconv/incubating/hw_metrics.h
around lines 737 to 747, the multiline string literal for descrMetricHwHostPower
is broken by an actual newline causing an unterminated-string compile error; fix
it by closing the quoted literal on the same line or using adjacent string
literals (e.g. "first part " "second part") so the message becomes a valid
single constant string, and ensure the trailing semicolon remains after the
declaration.
| Deprecated. Use @code zos @endcode instead. | ||
| @deprecated | ||
| {"note": "Replaced by @code zos @endcode.", "reason": "renamed", "renamed_to": "zos"} | ||
| */ | ||
| static constexpr const char *kZOs = "z_os"; | ||
| OPENTELEMETRY_DEPRECATED static constexpr const char *kZOs = "z_os"; | ||
|
|
There was a problem hiding this comment.
Fix regression: deprecated alias now returns an invalid token
kZOs used to expand to "zos", but this change flips it to "z_os". Any instrumentation still referencing the deprecated constant will now emit a non-spec value and stop matching back-end expectations. Keep the deprecated alias pointing at the canonical "zos" string until it’s removed.
-OPENTELEMETRY_DEPRECATED static constexpr const char *kZOs = "z_os";
+OPENTELEMETRY_DEPRECATED static constexpr const char *kZOs = "zos";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Deprecated. Use @code zos @endcode instead. | |
| @deprecated | |
| {"note": "Replaced by @code zos @endcode.", "reason": "renamed", "renamed_to": "zos"} | |
| */ | |
| static constexpr const char *kZOs = "z_os"; | |
| OPENTELEMETRY_DEPRECATED static constexpr const char *kZOs = "z_os"; | |
| Deprecated. Use @code zos @endcode instead. | |
| @deprecated | |
| {"note": "Replaced by @code zos @endcode.", "reason": "renamed", "renamed_to": "zos"} | |
| */ | |
| OPENTELEMETRY_DEPRECATED static constexpr const char *kZOs = "zos"; |
🤖 Prompt for AI Agents
In
deps/opentelemetry-cpp/api/include/opentelemetry/semconv/incubating/os_attributes.h
around lines 102-108 the deprecated alias kZOs was changed to the non-standard
token "z_os"; revert the alias to point at the canonical "zos" string literal so
deprecated references continue to emit the spec value (retain the
OPENTELEMETRY_DEPRECATED attribute and comment, only change the initializer back
to "zos").
| std::unique_ptr<opentelemetry::sdk::metrics::PushMetricExporter> ConsolePushMetricBuilder::Build( | ||
| const opentelemetry::sdk::configuration::ConsolePushMetricExporterConfiguration * /* model */) | ||
| const | ||
| { | ||
| return OStreamMetricExporterFactory::Create(); | ||
| } |
There was a problem hiding this comment.
Builder disregards all configuration data
Build ignores the supplied ConsolePushMetricExporterConfiguration, so none of the user-configurable options (e.g., ConsolePushMetricExporterOptions populated via model) ever reach OStreamMetricExporterFactory::Create. Any registry-driven customization (pretty print, custom stream, etc.) will be silently dropped. Please translate the configuration into exporter options before creating the exporter.
std::unique_ptr<opentelemetry::sdk::metrics::PushMetricExporter> ConsolePushMetricBuilder::Build(
- const opentelemetry::sdk::configuration::ConsolePushMetricExporterConfiguration * /* model */)
+ const opentelemetry::sdk::configuration::ConsolePushMetricExporterConfiguration *model)
const
{
- return OStreamMetricExporterFactory::Create();
+ opentelemetry::exporter::metrics::OStreamMetricExporterOptions options;
+ model->PopulateOptions(options);
+ return OStreamMetricExporterFactory::Create(options);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::unique_ptr<opentelemetry::sdk::metrics::PushMetricExporter> ConsolePushMetricBuilder::Build( | |
| const opentelemetry::sdk::configuration::ConsolePushMetricExporterConfiguration * /* model */) | |
| const | |
| { | |
| return OStreamMetricExporterFactory::Create(); | |
| } | |
| std::unique_ptr<opentelemetry::sdk::metrics::PushMetricExporter> ConsolePushMetricBuilder::Build( | |
| const opentelemetry::sdk::configuration::ConsolePushMetricExporterConfiguration *model) | |
| const | |
| { | |
| opentelemetry::exporter::metrics::OStreamMetricExporterOptions options; | |
| model->PopulateOptions(options); | |
| return OStreamMetricExporterFactory::Create(options); | |
| } |
| FilteringExcludeAttributesProcessor(FilterAttributeMap &&exclude_list = {}) | ||
| : exclude_list_(std::move(exclude_list)) | ||
| {} | ||
|
|
||
| FilteringExcludeAttributesProcessor(const FilterAttributeMap &exclude_list = {}) | ||
| : exclude_list_(exclude_list) | ||
| {} | ||
|
|
There was a problem hiding this comment.
Same constructor ambiguity here; fix with pass-by-value
Mirror the fix for the exclude variant to avoid ambiguous zero-arg constructor.
Apply:
-class FilteringExcludeAttributesProcessor : public AttributesProcessor
-{
-public:
- FilteringExcludeAttributesProcessor(FilterAttributeMap &&exclude_list = {})
- : exclude_list_(std::move(exclude_list))
- {}
-
- FilteringExcludeAttributesProcessor(const FilterAttributeMap &exclude_list = {})
- : exclude_list_(exclude_list)
- {}
+class FilteringExcludeAttributesProcessor : public AttributesProcessor
+{
+public:
+ FilteringExcludeAttributesProcessor() = default;
+ explicit FilteringExcludeAttributesProcessor(FilterAttributeMap exclude_list)
+ : exclude_list_(std::move(exclude_list))
+ {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FilteringExcludeAttributesProcessor(FilterAttributeMap &&exclude_list = {}) | |
| : exclude_list_(std::move(exclude_list)) | |
| {} | |
| FilteringExcludeAttributesProcessor(const FilterAttributeMap &exclude_list = {}) | |
| : exclude_list_(exclude_list) | |
| {} | |
| class FilteringExcludeAttributesProcessor : public AttributesProcessor | |
| { | |
| public: | |
| FilteringExcludeAttributesProcessor() = default; | |
| explicit FilteringExcludeAttributesProcessor(FilterAttributeMap exclude_list) | |
| : exclude_list_(std::move(exclude_list)) | |
| {} |
🤖 Prompt for AI Agents
In
deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/metrics/view/attributes_processor.h
around lines 122-129, the const FilterAttributeMap& constructor creates an
ambiguity with the rvalue ctor when using the default {}; change the
const-reference overload to take FilterAttributeMap by value (i.e.,
FilterAttributeMap exclude_list = {}) and move it into exclude_list_ in the
initializer (std::move(exclude_list)) so the zero-arg call is unambiguous and
mirrors the exclude variant fix.
| bool Shutdown( | ||
| std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept override | ||
| { | ||
| return InternalShutdown(timeout); | ||
| } | ||
|
|
||
| ~MultiSpanProcessor() override | ||
| { | ||
| InternalShutdown(); | ||
| Cleanup(); | ||
| } | ||
|
|
||
| protected: | ||
| bool InternalShutdown( | ||
| std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept |
There was a problem hiding this comment.
Restore cleanup in Shutdown() to honor span processor contract
By delegating Shutdown() solely to InternalShutdown(), we no longer clear the processor list. As a result, after a caller invokes Shutdown(), head_/tail_ stay populated, so any late OnStart/OnEnd calls still dispatch into processors that are supposed to be closed. The destructor will also re-run Shutdown() logic on still-linked processors. This is a regression from the previous behavior where Shutdown() performed both the per-processor shutdown and the Cleanup(). Please reintroduce the cleanup. For example:
bool Shutdown(
std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept override
{
- return InternalShutdown(timeout);
+ bool result = InternalShutdown(timeout);
+ Cleanup();
+ return result;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool Shutdown( | |
| std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept override | |
| { | |
| return InternalShutdown(timeout); | |
| } | |
| ~MultiSpanProcessor() override | |
| { | |
| InternalShutdown(); | |
| Cleanup(); | |
| } | |
| protected: | |
| bool InternalShutdown( | |
| std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept | |
| bool Shutdown( | |
| std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept override | |
| { | |
| bool result = InternalShutdown(timeout); | |
| Cleanup(); | |
| return result; | |
| } | |
| ~MultiSpanProcessor() override | |
| { | |
| InternalShutdown(); | |
| Cleanup(); | |
| } | |
| protected: | |
| bool InternalShutdown( | |
| std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept |
🤖 Prompt for AI Agents
In
deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/trace/multi_span_processor.h
around lines 124-138, Shutdown() currently delegates only to InternalShutdown()
and therefore does not clear head_/tail_ (Cleanup()), causing closed processors
to remain linked and receive late OnStart/OnEnd calls; update Shutdown() to call
Cleanup() after InternalShutdown() returns (preserving and returning the
InternalShutdown boolean result) so Shutdown performs both per-processor
shutdown and list cleanup (maintain noexcept and call order:
InternalShutdown(timeout) then Cleanup()).
| BatchLogRecordProcessor::~BatchLogRecordProcessor() | ||
| { | ||
| if (synchronization_data_->is_shutdown.load() == false) | ||
| { | ||
| InternalShutdown(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Destructor calls InternalShutdown() without an argument—verify default parameter exists; otherwise pass zero explicitly
If the header does not declare a default value for the timeout, this won’t compile. Safer to pass an explicit zero-duration timeout.
Apply this diff if there’s no default arg in the declaration:
- InternalShutdown();
+ InternalShutdown(std::chrono::microseconds::zero());Run the script to confirm the declaration has a default timeout:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the declaration of InternalShutdown and check for a default argument
rg -n -C2 -g 'deps/opentelemetry-cpp/**' $'\\bclass\\s+BatchLogRecordProcessor\\b'
rg -n -C2 -g 'deps/opentelemetry-cpp/**' $'\\bbool\\s+InternalShutdown\\s*\\(\\s*std::chrono::microseconds\\s+timeout(\\s*=\\s*[^)]*)?\\)\\s*noexcept\\s*;'Length of output: 613
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the declaration of InternalShutdown with surrounding context
rg -n -C2 'InternalShutdown' deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/logs/batch_log_record_processor.hLength of output: 190
Use explicit zero-duration timeout for InternalShutdown() in the destructor
The default parameter is std::chrono::microseconds::max(), causing the destructor to block indefinitely. Pass a zero timeout to ensure non-blocking shutdown:
--- a/deps/opentelemetry-cpp/sdk/src/logs/batch_log_record_processor.cc
+++ b/deps/opentelemetry-cpp/sdk/src/logs/batch_log_record_processor.cc
@@ -375,7 +375,7 @@ BatchLogRecordProcessor::~BatchLogRecordProcessor()
if (synchronization_data_->is_shutdown.load() == false)
{
- InternalShutdown();
+ InternalShutdown(std::chrono::microseconds::zero());
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| BatchLogRecordProcessor::~BatchLogRecordProcessor() | |
| { | |
| if (synchronization_data_->is_shutdown.load() == false) | |
| { | |
| InternalShutdown(); | |
| } | |
| } | |
| BatchLogRecordProcessor::~BatchLogRecordProcessor() | |
| { | |
| if (synchronization_data_->is_shutdown.load() == false) | |
| { | |
| InternalShutdown(std::chrono::microseconds::zero()); | |
| } | |
| } |
🤖 Prompt for AI Agents
In deps/opentelemetry-cpp/sdk/src/logs/batch_log_record_processor.cc around
lines 375 to 382, the destructor calls InternalShutdown() with its default
timeout (std::chrono::microseconds::max()) which can block indefinitely; change
the call to pass a zero-duration timeout (e.g., std::chrono::microseconds{0}) so
the destructor does a non-blocking shutdown, ensuring you construct and pass the
zero timeout value to InternalShutdown() only when is_shutdown is false.
| std::lock_guard<opentelemetry::common::SpinLockMutex> guard(attribute_hashmap_lock_); | ||
| delta_metrics = std::move(attributes_hashmap_); | ||
| attributes_hashmap_.reset(new AttributesHashMap); | ||
| attributes_hashmap_.reset(new AttributesHashMap(aggregation_config_->cardinality_limit_)); | ||
| } |
There was a problem hiding this comment.
Potential null dereference of aggregation_config_.
Collect unconditionally dereferences aggregation_config_. If the storage was constructed with a null config, this will crash. Normalize to a non‑null config via GetOrDefault before accessing the limit.
Apply this diff:
- delta_metrics = std::move(attributes_hashmap_);
- attributes_hashmap_.reset(new AttributesHashMap(aggregation_config_->cardinality_limit_));
+ delta_metrics = std::move(attributes_hashmap_);
+ auto agg_cfg = AggregationConfig::GetOrDefault(aggregation_config_);
+ attributes_hashmap_.reset(new AttributesHashMap(agg_cfg->cardinality_limit_));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::lock_guard<opentelemetry::common::SpinLockMutex> guard(attribute_hashmap_lock_); | |
| delta_metrics = std::move(attributes_hashmap_); | |
| attributes_hashmap_.reset(new AttributesHashMap); | |
| attributes_hashmap_.reset(new AttributesHashMap(aggregation_config_->cardinality_limit_)); | |
| } | |
| std::lock_guard<opentelemetry::common::SpinLockMutex> guard(attribute_hashmap_lock_); | |
| delta_metrics = std::move(attributes_hashmap_); | |
| auto agg_cfg = AggregationConfig::GetOrDefault(aggregation_config_); | |
| attributes_hashmap_.reset(new AttributesHashMap(agg_cfg->cardinality_limit_)); | |
| } |
🤖 Prompt for AI Agents
In deps/opentelemetry-cpp/sdk/src/metrics/state/sync_metric_storage.cc around
lines 37 to 40, the code unconditionally dereferences aggregation_config_ to
access cardinality_limit_, which can be null and cause a crash; replace the
direct use with a normalized non-null config obtained via GetOrDefault (or
equivalent helper) and use that config's cardinality_limit_ when constructing
the new AttributesHashMap so the code never dereferences a null pointer.
| setenv(env_interval, "1500ms", 1); | ||
| setenv(env_timeout, "1000ms", 1); | ||
|
|
||
| PeriodicExportingMetricReaderOptions options; | ||
| EXPECT_EQ(options.export_interval_millis, std::chrono::milliseconds(1500)); | ||
| EXPECT_EQ(options.export_timeout_millis, std::chrono::milliseconds(1000)); | ||
|
|
||
| unsetenv(env_interval); | ||
| unsetenv(env_timeout); | ||
| } |
There was a problem hiding this comment.
Call unsetenv through the Windows shim as well
Great to see the platform guard for setenv. Please mirror that for unsetenv—on MSVC we need to go through the shim from env_variables.h, while on POSIX we should stick with the libc version. Right now the call inside the test always hits the global unsetenv, which doesn’t exist under MSVC and will blow up the Windows build.
🤖 Prompt for AI Agents
In
deps/opentelemetry-cpp/sdk/test/metrics/periodic_exporting_metric_reader_test.cc
around lines 125 to 134, the test calls unsetenv directly which fails on MSVC;
mirror the existing platform-guarded setenv usage and call the unsetenv shim
from env_variables.h on Windows and libc unsetenv on POSIX. Replace direct
unsetenv(env_interval)/unsetenv(env_timeout) with the same #if/#else guard used
for setenv so the Windows build uses the shim and POSIX continues to use the
libc function.
| static void Register(opentelemetry::sdk::configuration::Registry *registry); | ||
|
|
||
| std::unique_ptr<opentelemetry::sdk::metrics::PushMetricExporter> Build( | ||
| const opentelemetry::sdk::configuration::OtlpFilePushMetricExporterConfiguration *model) | ||
| const override; |
There was a problem hiding this comment.
🧩 Analysis chain
Ensure the new builder is actually wired into the registry.
Line 24: Please confirm that some startup path calls OtlpFilePushMetricBuilder::Register(...); otherwise the file push metric exporter never becomes discoverable. A quick search should reveal the registration site.
🏁 Script executed:
#!/bin/bash
# Verify that OtlpFilePushMetricBuilder::Register is invoked somewhere
rg -n "OtlpFilePushMetricBuilder::Register"Length of output: 173
Wire OtlpFilePushMetricBuilder into registry
No calls to OtlpFilePushMetricBuilder::Register were found (only its definition in otlp_file_push_metric_builder.cc); add a call to Register(registry) in the startup path (e.g., exporter initialization) so the file push metric exporter becomes discoverable.
🤖 Prompt for AI Agents
In
deps/opentelemetry-cpp/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_file_push_metric_builder.h
around lines 24-28, the OtlpFilePushMetricBuilder is declared but never wired
into the registry; add a call to OtlpFilePushMetricBuilder::Register(registry)
in the application's exporter startup/initialization path (for example where
other exporters are registered or the exporter registry is constructed) so the
file push metric exporter is discoverable; ensure this registration is invoked
during startup before exporter discovery is performed and follow existing
registration patterns (error-check/guard if registry is null) used by other
exporter builders.
| void OtlpHttpSpanBuilder::Register(opentelemetry::sdk::configuration::Registry *registry) | ||
| { | ||
| auto builder = std::make_unique<OtlpHttpSpanBuilder>(); | ||
| registry->SetOtlpHttpSpanBuilder(std::move(builder)); | ||
| } |
There was a problem hiding this comment.
Guard registry pointer in Register.
void OtlpHttpSpanBuilder::Register(opentelemetry::sdk::configuration::Registry *registry)
{
+ assert(registry != nullptr);
auto builder = std::make_unique<OtlpHttpSpanBuilder>();
registry->SetOtlpHttpSpanBuilder(std::move(builder));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void OtlpHttpSpanBuilder::Register(opentelemetry::sdk::configuration::Registry *registry) | |
| { | |
| auto builder = std::make_unique<OtlpHttpSpanBuilder>(); | |
| registry->SetOtlpHttpSpanBuilder(std::move(builder)); | |
| } | |
| void OtlpHttpSpanBuilder::Register(opentelemetry::sdk::configuration::Registry *registry) | |
| { | |
| assert(registry != nullptr); | |
| auto builder = std::make_unique<OtlpHttpSpanBuilder>(); | |
| registry->SetOtlpHttpSpanBuilder(std::move(builder)); | |
| } |
🤖 Prompt for AI Agents
In deps/opentelemetry-cpp/exporters/otlp/src/otlp_http_span_builder.cc around
lines 26 to 30, the Register function dereferences the registry pointer without
validation; add a null check at the start (if registry == nullptr) and return
early (or log and return) to avoid dereferencing a null pointer, then proceed to
create the builder and call SetOtlpHttpSpanBuilder only when registry is
non-null.
| std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> OtlpHttpSpanBuilder::Build( | ||
| const opentelemetry::sdk::configuration::OtlpHttpSpanExporterConfiguration *model) const | ||
| { | ||
| OtlpHttpExporterOptions options(nullptr); | ||
|
|
||
| options.url = model->endpoint; | ||
| options.content_type = OtlpBuilderUtils::ConvertOtlpHttpEncoding(model->encoding); | ||
| options.json_bytes_mapping = JsonBytesMappingKind::kHexId; | ||
| options.use_json_name = false; | ||
| options.console_debug = false; | ||
| options.timeout = std::chrono::duration_cast<std::chrono::system_clock::duration>( | ||
| std::chrono::seconds{model->timeout}); | ||
| options.http_headers = | ||
| OtlpBuilderUtils::ConvertHeadersConfigurationModel(model->headers.get(), model->headers_list); | ||
| options.ssl_insecure_skip_verify = false; | ||
| options.ssl_ca_cert_path = model->certificate_file; | ||
| options.ssl_client_key_path = model->client_key_file; | ||
| options.ssl_client_cert_path = model->client_certificate_file; | ||
| options.compression = model->compression; | ||
|
|
||
| return OtlpHttpExporterFactory::Create(options); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Null-deref risk and preserving option defaults.
Build dereferences model and overwrites defaults even when config fields are empty. Add checks to avoid NPE and preserve default behaviors (env/configured defaults).
std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> OtlpHttpSpanBuilder::Build(
const opentelemetry::sdk::configuration::OtlpHttpSpanExporterConfiguration *model) const
{
- OtlpHttpExporterOptions options(nullptr);
-
- options.url = model->endpoint;
+ assert(model != nullptr);
+ if (model == nullptr)
+ {
+ return nullptr;
+ }
+ OtlpHttpExporterOptions options(nullptr);
+
+ if (!model->endpoint.empty())
+ {
+ options.url = model->endpoint;
+ }
options.content_type = OtlpBuilderUtils::ConvertOtlpHttpEncoding(model->encoding);
options.json_bytes_mapping = JsonBytesMappingKind::kHexId;
options.use_json_name = false;
options.console_debug = false;
- options.timeout = std::chrono::duration_cast<std::chrono::system_clock::duration>(
- std::chrono::seconds{model->timeout});
+ if (model->timeout > 0)
+ {
+ options.timeout = std::chrono::duration_cast<std::chrono::system_clock::duration>(
+ std::chrono::seconds{model->timeout});
+ }
options.http_headers =
OtlpBuilderUtils::ConvertHeadersConfigurationModel(model->headers.get(), model->headers_list);
options.ssl_insecure_skip_verify = false;
- options.ssl_ca_cert_path = model->certificate_file;
- options.ssl_client_key_path = model->client_key_file;
- options.ssl_client_cert_path = model->client_certificate_file;
+ if (!model->certificate_file.empty())
+ options.ssl_ca_cert_path = model->certificate_file;
+ if (!model->client_key_file.empty())
+ options.ssl_client_key_path = model->client_key_file;
+ if (!model->client_certificate_file.empty())
+ options.ssl_client_cert_path = model->client_certificate_file;
options.compression = model->compression;
return OtlpHttpExporterFactory::Create(options);
}- Confirm that leaving url/timeout unset triggers expected defaults (env vars or library defaults).
- Confirm ConvertHeadersConfigurationModel handles null headers pointer.
🏁 Script executed:
#!/bin/bash
# Locate OtlpHttpExporterOptions structure and its constructor
rg -n "class OtlpHttpExporterOptions" -C5
rg -n "OtlpHttpExporterOptions::OtlpHttpExporterOptions" -C5
# Inspect default handling in OtlpHttpExporterFactory::Create
rg -n "OtlpHttpExporterFactory::Create" -C10
# Find ConvertHeadersConfigurationModel declaration/definition
rg -n "ConvertHeadersConfigurationModel" -C10Length of output: 38024
🏁 Script executed:
#!/bin/bash
# Display ConvertHeadersConfigurationModel implementation to check null handling of model pointer.
sed -n '40,120p' deps/opentelemetry-cpp/exporters/otlp/src/otlp_builder_utils.ccLength of output: 2230
🏁 Script executed:
#!/bin/bash
# Locate OtlpHttpExporter class and its constructor
rg -n "class OtlpHttpExporter" -C5 deps/opentelemetry-cpp/exporters/otlp/src/otlp_http_exporter.h
rg -n "OtlpHttpExporter::OtlpHttpExporter" -C10 deps/opentelemetry-cpp/exporters/otlp/src/otlp_http_exporter.ccLength of output: 3490
🏁 Script executed:
#!/bin/bash
# Search for OtlpHttpSpanExporterConfiguration definition to check its default values.
rg -n "struct OtlpHttpSpanExporterConfiguration" -C5 deps/opentelemetry-cppLength of output: 0
🏁 Script executed:
#!/bin/bash
# Show includes and context around configuration model usage in span builder.
sed -n '1,60p' deps/opentelemetry-cpp/exporters/otlp/src/otlp_http_span_builder.ccLength of output: 2232
🏁 Script executed:
#!/bin/bash
# Locate and display the configuration builder to check default initialization of model fields.
file=$(fd -t f otlp_http_span_exporter_builder.* deps/opentelemetry-cpp | head -n1)
echo "Builder file: $file"
sed -n '1,200p' "$file"Length of output: 1329
🏁 Script executed:
#!/bin/bash
sed -n '1,240p' deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/registry.h
rg -n "SetOtlpHttpSpanBuilder|GetOtlpHttpSpanBuilder|OtlpHttpSpanBuilder" deps/opentelemetry-cpp -SLength of output: 9417
🏁 Script executed:
#!/bin/bash
# Inspect OtlpHttpSpanExporterConfiguration fields and defaults
sed -n '1,200p' deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/otlp_http_span_exporter_configuration.h
sed -n '200,400p' deps/opentelemetry-cpp/sdk/include/opentelemetry/sdk/configuration/otlp_http_span_exporter_configuration.hLength of output: 1184
🏁 Script executed:
#!/bin/bash
# Locate and display SpanExporterConfigurationVisitor implementation for HTTP spans.
rg -n "class SpanExporterConfigurationVisitor" -C5 deps/opentelemetry-cpp/sdk/src/configuration
file=$(rg -l "span_exporter_configuration_visitor" -g "*.cc" deps/opentelemetry-cpp/sdk/src/configuration | head -n1)
echo "Visitor file: $file"
sed -n '1,200p' "$file"Length of output: 12595
Guard null pointers & preserve defaults
- In
Build(), verifymodelandmodel->headersbefore use (avoid dereferencing null). - Only assign
options.url,options.timeout,ssl_*paths andhttp_headerswhen the correspondingmodelfields are set (non-empty/timeout>0) so default values (from constructor or environment) remain intact.
🤖 Prompt for AI Agents
In deps/opentelemetry-cpp/exporters/otlp/src/otlp_http_span_builder.cc around
lines 32 to 53, Build() currently dereferences model and model->headers and
unconditionally overwrites all options; guard against a null model pointer and
null model->headers before dereferencing, and only assign options.url,
options.timeout, options.ssl_ca_cert_path, options.ssl_client_key_path,
options.ssl_client_cert_path and options.http_headers when the corresponding
model fields are present (e.g., model != nullptr, strings are non-empty, timeout
> 0) so constructor/environment defaults are preserved; perform a null-check on
model at function start (return or handle error), check model->headers before
calling headers.get(), and wrap each optional assignment in a conditional that
verifies the model field is set.
| void ZipkinBuilder::Register(opentelemetry::sdk::configuration::Registry *registry) | ||
| { | ||
| auto builder = std::make_unique<ZipkinBuilder>(); | ||
| registry->SetZipkinSpanBuilder(std::move(builder)); | ||
| } |
There was a problem hiding this comment.
Guard registry pointer in Register.
Defensive programming: assert non-null to avoid silent no-ops or crashes.
void ZipkinBuilder::Register(opentelemetry::sdk::configuration::Registry *registry)
{
+ assert(registry != nullptr);
auto builder = std::make_unique<ZipkinBuilder>();
registry->SetZipkinSpanBuilder(std::move(builder));
}🤖 Prompt for AI Agents
In deps/opentelemetry-cpp/exporters/zipkin/src/zipkin_builder.cc around lines 23
to 27, the Register function uses the registry pointer without checking it; add
a null check (or an assert) at the start of the function to guard against a null
registry and avoid silent no-ops or crashes, e.g. assert(registry) or if
(!registry) return (or log/handle error) before creating and setting the
ZipkinBuilder.
| std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> ZipkinBuilder::Build( | ||
| const opentelemetry::sdk::configuration::ZipkinSpanExporterConfiguration *model) const | ||
| { | ||
| ZipkinExporterOptions options(nullptr); | ||
|
|
||
| options.endpoint = model->endpoint; | ||
|
|
||
| return ZipkinExporterFactory::Create(options); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Null-deref risk and default endpoint clobbering.
Build unconditionally dereferences model and overwrites defaults with possibly-empty endpoint.
std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> ZipkinBuilder::Build(
const opentelemetry::sdk::configuration::ZipkinSpanExporterConfiguration *model) const
{
- ZipkinExporterOptions options(nullptr);
-
- options.endpoint = model->endpoint;
+ assert(model != nullptr);
+ if (model == nullptr)
+ {
+ return nullptr;
+ }
+ ZipkinExporterOptions options(nullptr);
+ if (!model->endpoint.empty())
+ {
+ options.endpoint = model->endpoint;
+ }
return ZipkinExporterFactory::Create(options);
}Please confirm whether ZipkinExporterOptions sets a sensible default endpoint (e.g., via env) and whether empty strings should preserve that default.
🏁 Script executed:
#!/bin/bash
# Locate the definition of ZipkinExporterOptions
rg -n "struct ZipkinExporterOptions" -C5 .
# Inspect its constructor(s) for default endpoint behavior
rg -n "ZipkinExporterOptions::ZipkinExporterOptions" -C5 .
# Find where endpoint is used or overridden in the factory
rg -n "ZipkinExporterFactory::Create" -C5 deps/opentelemetry-cpp/exporters/zipkinLength of output: 4049
🏁 Script executed:
#!/bin/bash
# Search all constructors of ZipkinExporterOptions and usages
rg -n "ZipkinExporterOptions\s*\(" -C3 deps/opentelemetry-cpp/exporters/zipkin
# Locate GetDefaultZipkinEndpoint implementation
rg -n "GetDefaultZipkinEndpoint" -C5 deps/opentelemetry-cpp/exporters/zipkinLength of output: 5730
Guard null model and preserve default endpoint
- Assert or check
model != nullptr(e.g.assert(model); if (!model) return nullptr;) before dereferencing. - Instantiate with the default ctor (
ZipkinExporterOptions options;) soGetDefaultZipkinEndpoint()is used, then only do
if (!model->endpoint.empty()) options.endpoint = model->endpoint;
to avoid clobbering the default whenmodel->endpointis empty.
🤖 Prompt for AI Agents
In deps/opentelemetry-cpp/exporters/zipkin/src/zipkin_builder.cc around lines 29
to 37, guard against a null model before dereferencing (e.g. assert(model); if
(!model) return nullptr;) and instantiate ZipkinExporterOptions with the default
constructor so the default endpoint is preserved; then only override
options.endpoint when model->endpoint is non-empty (if
(!model->endpoint.empty()) options.endpoint = model->endpoint;) before calling
ZipkinExporterFactory::Create(options).
| target_link_libraries( | ||
| opentelemetry_http_client_curl | ||
| PUBLIC opentelemetry_ext | ||
| PRIVATE CURL::libcurl) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify CURL imported target availability.
Linking to CURL::libcurl assumes find_package(CURL) defines this target; verify top-level CMake does so on all supported platforms.
🏁 Script executed:
#!/bin/bash
# Scan CMake for find_package calls and CURL/ZLIB usage.
rg -n -C2 -g '!**/build/**' -P 'find_package\((CURL|ZLIB)[^)]*\)|CURL::libcurl|ZLIB::ZLIB|WITH_OTLP_HTTP_COMPRESSION' CMakeLists.txt deps/opentelemetry-cpp -SLength of output: 54
🏁 Script executed:
#!/bin/bash
# List all CMakeLists.txt files to locate project roots
find . -type f -name CMakeLists.txt
# Search across all .txt and .cmake files for find_package(CURL and find_package(ZLIB)
rg -n -C2 'find_package\(\s*CURL' -g '*.txt' -g '*.cmake'
rg -n -C2 'find_package\(\s*ZLIB' -g '*.txt' -g '*.cmake'
# Verify any occurrences of the imported target
rg -n 'CURL::libcurl' -g '*.txt' -g '*.cmake'Length of output: 6952
Add find_package(CURL REQUIRED) before linking to CURL::libcurl
The root CMakeLists.txt doesn’t invoke find_package(CURL), yet targets link to CURL::libcurl; add find_package(CURL REQUIRED) (e.g. in the top‐level CMakeLists.txt) to ensure the imported target is defined on all platforms.
🤖 Prompt for AI Agents
In deps/opentelemetry-cpp/ext/src/http/client/curl/CMakeLists.txt around lines
14-17, the target links to the imported target CURL::libcurl but no
find_package(CURL REQUIRED) is invoked anywhere, so the imported target may be
undefined on some platforms; add a find_package(CURL REQUIRED) call (preferably
in the top-level/root CMakeLists.txt or immediately before the
target_link_libraries call) so the CURL package is discovered and CURL::libcurl
is available before linking.
| target_link_libraries( | ||
| opentelemetry_http_client_curl | ||
| PUBLIC opentelemetry_ext | ||
| PRIVATE ZLIB::ZLIB) |
There was a problem hiding this comment.
Build break risk: unconditional ZLIB linkage without ensuring it exists.
When WITH_OTLP_HTTP_COMPRESSION is ON, ZLIB::ZLIB must be discoverable; add find_package(ZLIB REQUIRED) and drop duplicate opentelemetry_ext linkage.
-if(WITH_OTLP_HTTP_COMPRESSION)
- target_link_libraries(
- opentelemetry_http_client_curl
- PUBLIC opentelemetry_ext
- PRIVATE ZLIB::ZLIB)
-endif()
+if(WITH_OTLP_HTTP_COMPRESSION)
+ find_package(ZLIB REQUIRED)
+ target_link_libraries(opentelemetry_http_client_curl PRIVATE ZLIB::ZLIB)
+endif()Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In deps/opentelemetry-cpp/ext/src/http/client/curl/CMakeLists.txt around lines
25-28, the target unconditionally links ZLIB::ZLIB which can break builds if
zlib wasn't found; modify the CMake to call find_package(ZLIB REQUIRED) when
WITH_OTLP_HTTP_COMPRESSION is ON and only link ZLIB::ZLIB inside that
conditional, and remove the duplicate PUBLIC opentelemetry_ext linkage if it's
already linked elsewhere (keep only one opentelemetry_ext linkage and add
PRIVATE ZLIB::ZLIB only when find_package succeeds).
|
|
||
| TEST(AttributesHashMap, OverflowCardinalityLimitBehavior) | ||
| { | ||
| // Configure a very small limit to exercise overflow logic easily. | ||
| const size_t limit = 4; // real attributes limit | ||
| AttributesHashMapWithCustomHash<> map(limit); | ||
|
|
||
| // We expect to be able to insert exactly 'limit' distinct real attribute sets. | ||
| // After that, further distinct attributes should route to the overflow bucket, | ||
| // which should appear only once regardless of how many additional unique sets arrive. | ||
|
|
||
| // Insert distinct attributes up to the limit. | ||
| for (size_t i = 0; i < limit; ++i) | ||
| { | ||
| MetricAttributes attr = {{"k", std::to_string(i)}}; | ||
| map.GetOrSetDefault(attr, []() { return std::unique_ptr<Aggregation>(new DropAggregation()); }) | ||
| ->Aggregate(static_cast<int64_t>(1)); | ||
| } | ||
|
|
||
| // Size should be exactly 'limit' (no overflow yet) | ||
| EXPECT_EQ(map.Size(), limit); | ||
|
|
||
| // Insert one more distinct attribute; this should not increase the real attributes count | ||
| MetricAttributes overflow_trigger = {{"k", "overflow"}}; | ||
| map.GetOrSetDefault(overflow_trigger, | ||
| []() { return std::unique_ptr<Aggregation>(new DropAggregation()); }) | ||
| ->Aggregate(static_cast<int64_t>(1)); | ||
|
|
||
| EXPECT_EQ(map.Size(), limit); | ||
|
|
||
| // Insert several more unique attributes - size must remain constant (limit) | ||
| for (size_t i = 0; i < limit - 1; ++i) | ||
| { | ||
| MetricAttributes extra_attr = {{"k", std::string("extra") + std::to_string(i)}}; | ||
| map.GetOrSetDefault(extra_attr, | ||
| []() { return std::unique_ptr<Aggregation>(new DropAggregation()); }) | ||
| ->Aggregate(static_cast<int64_t>(1)); | ||
| } | ||
| EXPECT_EQ(map.Size(), limit); | ||
|
|
||
| // Ensure overflow key was actually created and accessible via Get | ||
| EXPECT_NE(map.Get(kOverflowAttributes), nullptr); | ||
|
|
||
| // Ensure original real attributes still present | ||
| for (size_t i = 0; i < limit - 1; ++i) | ||
| { | ||
| MetricAttributes attr = {{"k", std::to_string(i)}}; | ||
| EXPECT_NE(map.Get(attr), nullptr); | ||
| } | ||
|
|
||
| // Copy the hash map to a new map in non-determistic order and verify all entries are present | ||
| AttributesHashMapWithCustomHash<> map_copy(limit); | ||
| map.GetAllEnteries([&map_copy](const MetricAttributes &attributes, Aggregation &) { | ||
| map_copy.Set(attributes, std::unique_ptr<Aggregation>(new DropAggregation())); | ||
| return true; | ||
| }); | ||
| EXPECT_EQ(map_copy.Size(), map.Size()); | ||
| EXPECT_NE(map_copy.Get(kOverflowAttributes), nullptr); | ||
| for (size_t i = 0; i < limit - 1; ++i) | ||
| { | ||
| MetricAttributes attr = {{"k", std::to_string(i)}}; | ||
| EXPECT_NE(map_copy.Get(attr), nullptr); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid brittle assumptions about which real entry is evicted
The test assumes specific preserved keys after overflow (checking only 0..limit-2). Eviction policy or insertion order is not guaranteed, so this can be flaky. Count how many of the initial keys remain instead of asserting on specific ones. Also fix the typo “non-determistic”.
Apply this diff:
- // Ensure original real attributes still present
- for (size_t i = 0; i < limit - 1; ++i)
- {
- MetricAttributes attr = {{"k", std::to_string(i)}};
- EXPECT_NE(map.Get(attr), nullptr);
- }
+ // Ensure exactly (limit - 1) of the original real attributes still present
+ size_t present_count = 0;
+ for (size_t i = 0; i < limit; ++i)
+ {
+ MetricAttributes attr = {{"k", std::to_string(i)}};
+ present_count += (map.Get(attr) != nullptr);
+ }
+ EXPECT_NE(map.Get(kOverflowAttributes), nullptr);
+ EXPECT_EQ(present_count, limit - 1);
@@
- // Copy the hash map to a new map in non-determistic order and verify all entries are present
+ // Copy the hash map to a new map in non-deterministic order and verify all entries are present
@@
- for (size_t i = 0; i < limit - 1; ++i)
- {
- MetricAttributes attr = {{"k", std::to_string(i)}};
- EXPECT_NE(map_copy.Get(attr), nullptr);
- }
+ size_t copy_present_count = 0;
+ for (size_t i = 0; i < limit; ++i)
+ {
+ MetricAttributes attr = {{"k", std::to_string(i)}};
+ copy_present_count += (map_copy.Get(attr) != nullptr);
+ }
+ EXPECT_EQ(copy_present_count, limit - 1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TEST(AttributesHashMap, OverflowCardinalityLimitBehavior) | |
| { | |
| // Configure a very small limit to exercise overflow logic easily. | |
| const size_t limit = 4; // real attributes limit | |
| AttributesHashMapWithCustomHash<> map(limit); | |
| // We expect to be able to insert exactly 'limit' distinct real attribute sets. | |
| // After that, further distinct attributes should route to the overflow bucket, | |
| // which should appear only once regardless of how many additional unique sets arrive. | |
| // Insert distinct attributes up to the limit. | |
| for (size_t i = 0; i < limit; ++i) | |
| { | |
| MetricAttributes attr = {{"k", std::to_string(i)}}; | |
| map.GetOrSetDefault(attr, []() { return std::unique_ptr<Aggregation>(new DropAggregation()); }) | |
| ->Aggregate(static_cast<int64_t>(1)); | |
| } | |
| // Size should be exactly 'limit' (no overflow yet) | |
| EXPECT_EQ(map.Size(), limit); | |
| // Insert one more distinct attribute; this should not increase the real attributes count | |
| MetricAttributes overflow_trigger = {{"k", "overflow"}}; | |
| map.GetOrSetDefault(overflow_trigger, | |
| []() { return std::unique_ptr<Aggregation>(new DropAggregation()); }) | |
| ->Aggregate(static_cast<int64_t>(1)); | |
| EXPECT_EQ(map.Size(), limit); | |
| // Insert several more unique attributes - size must remain constant (limit) | |
| for (size_t i = 0; i < limit - 1; ++i) | |
| { | |
| MetricAttributes extra_attr = {{"k", std::string("extra") + std::to_string(i)}}; | |
| map.GetOrSetDefault(extra_attr, | |
| []() { return std::unique_ptr<Aggregation>(new DropAggregation()); }) | |
| ->Aggregate(static_cast<int64_t>(1)); | |
| } | |
| EXPECT_EQ(map.Size(), limit); | |
| // Ensure overflow key was actually created and accessible via Get | |
| EXPECT_NE(map.Get(kOverflowAttributes), nullptr); | |
| // Ensure original real attributes still present | |
| for (size_t i = 0; i < limit - 1; ++i) | |
| { | |
| MetricAttributes attr = {{"k", std::to_string(i)}}; | |
| EXPECT_NE(map.Get(attr), nullptr); | |
| } | |
| // Copy the hash map to a new map in non-determistic order and verify all entries are present | |
| AttributesHashMapWithCustomHash<> map_copy(limit); | |
| map.GetAllEnteries([&map_copy](const MetricAttributes &attributes, Aggregation &) { | |
| map_copy.Set(attributes, std::unique_ptr<Aggregation>(new DropAggregation())); | |
| return true; | |
| }); | |
| EXPECT_EQ(map_copy.Size(), map.Size()); | |
| EXPECT_NE(map_copy.Get(kOverflowAttributes), nullptr); | |
| for (size_t i = 0; i < limit - 1; ++i) | |
| { | |
| MetricAttributes attr = {{"k", std::to_string(i)}}; | |
| EXPECT_NE(map_copy.Get(attr), nullptr); | |
| } | |
| } | |
| // Ensure overflow key was actually created and accessible via Get | |
| EXPECT_NE(map.Get(kOverflowAttributes), nullptr); | |
| // Ensure exactly (limit - 1) of the original real attributes still present | |
| size_t present_count = 0; | |
| for (size_t i = 0; i < limit; ++i) | |
| { | |
| MetricAttributes attr = {{"k", std::to_string(i)}}; | |
| present_count += (map.Get(attr) != nullptr); | |
| } | |
| EXPECT_EQ(present_count, limit - 1); | |
| // Copy the hash map to a new map in non-deterministic order and verify all entries are present | |
| AttributesHashMapWithCustomHash<> map_copy(limit); | |
| map.GetAllEnteries([&map_copy](const MetricAttributes &attributes, Aggregation &) { | |
| map_copy.Set(attributes, std::unique_ptr<Aggregation>(new DropAggregation())); | |
| return true; | |
| }); | |
| EXPECT_EQ(map_copy.Size(), map.Size()); | |
| EXPECT_NE(map_copy.Get(kOverflowAttributes), nullptr); | |
| size_t copy_present_count = 0; | |
| for (size_t i = 0; i < limit; ++i) | |
| { | |
| MetricAttributes attr = {{"k", std::to_string(i)}}; | |
| copy_present_count += (map_copy.Get(attr) != nullptr); | |
| } | |
| EXPECT_EQ(copy_present_count, limit - 1); |
🤖 Prompt for AI Agents
In deps/opentelemetry-cpp/sdk/test/metrics/attributes_hashmap_test.cc around
lines 161-224, the test brittlely assumes which original entries survive
eviction by checking specific keys 0..limit-2 and contains a typo
"non-determistic"; change the assertions to count how many of the initial
distinct keys (0..limit-1) are still present (expect at least one overflow
bucket and that the number of surviving initial keys is <= limit and >= 0 as
appropriate — concretely, compute an integer counter by calling map.Get for each
initial key and expect counter > 0 or expect counter == expected_survivors based
on limit semantics), replace the individual EXPECT_NE checks with that
count-based EXPECT_EQ/EXPECT_GE check, and fix the comment typo to
"non-deterministic" (and the same counting change for map_copy verification).
2 participants
Summary by CodeRabbit
New Features
Improvements
Deprecations
Chores