Update OtlpGrpcClientOptions to populate options for gRPC client sharing#4249
Update OtlpGrpcClientOptions to populate options for gRPC client sharing#4249ML-dev-crypto wants to merge 4 commits into
Conversation
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4249 +/- ##
==========================================
+ Coverage 77.92% 78.15% +0.24%
==========================================
Files 439 440 +1
Lines 18612 18754 +142
==========================================
+ Hits 14502 14656 +154
+ Misses 4110 4098 -12
🚀 New features to boost your workflow:
|
|
Thanks for the thorough review, @owent! I'll address all of these comments in a follow-up commit, including the export annotation, constructor initialization cleanup, shared environment variable definitions, removal of the duplicated header parsing logic, moving the new environment helpers out of the header, reusing the base constructor for shared client initialization, and preserving the shared client's |
| namespace otlp | ||
| { | ||
|
|
||
| constexpr char kEnvOtlpEndpoint[] = "OTEL_EXPORTER_OTLP_ENDPOINT"; |
There was a problem hiding this comment.
Before C++17, a constexpr variable declared at namespace scope may have linkage symbol, so it emits an exported symbol. Please declare these as static constexpr char to give them internal linkage and avoid leaking symbols.
| { | ||
| std::string endpoint = GetOtlpDefaultGrpcClientEndpoint(); | ||
|
|
||
| if (endpoint.substr(0, 6) == "https:") |
There was a problem hiding this comment.
Just a suggestion, use 'nostd::string_view' here can slightly improve performance.
owent
left a comment
There was a problem hiding this comment.
Just a few minor non-blocking issues left — otherwise LGTM. Could you please resolve the conflicts?
Fixes open-telemetry#4239 OtlpGrpcClientOptions previously only had a = default constructor, leaving every option at null/zero. Add a real default constructor that populates spec-compliant defaults from generic (signal-independent) OTEL_EXPORTER_OTLP_* environment variables, a void* constructor that skips defaults, and a constructor on each of OtlpGrpcExporterOptions, OtlpGrpcMetricExporterOptions, and OtlpGrpcLogRecordExporterOptions that builds from a shared OtlpGrpcClientOptions, copying client-fixed fields and overriding only timeout, metadata, and max_concurrent_requests per signal. Signed-off-by: Ansh Rai <anshrai331@gmail.com>
Signed-off-by: Ansh Rai <anshrai331@gmail.com>
Signed-off-by: Ansh Rai <anshrai331@gmail.com>
Signed-off-by: Ansh Rai <anshrai331@gmail.com>
3afcbb1 to
2ca8d8e
Compare
|
@owent Thanks! Addressed the remaining comments, rebased onto the latest main, resolved the merge conflict, and pushed the updates. |
|
@owent |
Fixes #4239
OtlpGrpcClientOptionspreviously only had a= defaultconstructor,leaving every option at null/zero regardless of environment variables
or otel-spec defaults. This made it impossible to build a standalone
OtlpGrpcClientwith spec-compliant defaults for sharing across thetrace/metric/log exporters, as described in the issue.
Changes:
accessors to
otlp_environment.h, e.g.GetOtlpDefaultGrpcClientEndpoint().These are
inline, defined in the header rather thanotlp_environment.cc, so thatotlp_grpc_client(a lean transport-onlytarget) doesn't need to depend on
otlp_recordableand itssdk/logs, sdk/metrics, sdk/trace dependencies just to read an env var.
OtlpGrpcClientOptionsnow has a real default constructor (in a newotlp_grpc_client_options.cc) that populates from those accessors,and a
void*constructor that skips populating, for parity with theexisting per-signal options classes.
OtlpGrpcExporterOptions/OtlpGrpcMetricExporterOptions/OtlpGrpcLogRecordExporterOptionseach gained a constructor taking aconst OtlpGrpcClientOptions&, copying the client-fixed fields(endpoint, SSL/mTLS fields, credentials, user_agent, channel_arguments,
max_threads, compression, retry policy) and overriding only the fields
that may still differ per signal (timeout, metadata,
max_concurrent_requests) — per the table in the issue.
(
//sdk/src/common:env_variables); no new library targets, no changeto the existing dependency graph shape.
otlp_grpc_client_options_test.cccovering: defaultenv-var population, the
void*no-defaults path (including thederived classes'
void*constructors), fixed-field copying from ashared client, and signal-specific env-var precedence over the
shared client's generic value.
Tested: compiled and unit-tested with both GCC/libstdc++ and
MSVC/STL;
clang-formatapplied.