Update OtlpGrpcClientOptions to populate options for gRPC client sharing by ML-dev-crypto · Pull Request #4249 · open-telemetry/opentelemetry-cpp

ML-dev-crypto · 2026-07-15T09:51:05Z

OtlpGrpcClientOptions previously only had a = default constructor,
leaving every option at null/zero regardless of environment variables
or otel-spec defaults. This made it impossible to build a standalone
OtlpGrpcClient with spec-compliant defaults for sharing across the
trace/metric/log exporters, as described in the issue.

Changes:

Added signal-independent (no TRACES_/METRICS_/LOGS_ prefix) env-var
accessors to otlp_environment.h, e.g. GetOtlpDefaultGrpcClientEndpoint().
These are inline, defined in the header rather than
otlp_environment.cc, so that otlp_grpc_client (a lean transport-only
target) doesn't need to depend on otlp_recordable and its
sdk/logs, sdk/metrics, sdk/trace dependencies just to read an env var.
OtlpGrpcClientOptions now has a real default constructor (in a new
otlp_grpc_client_options.cc) that populates from those accessors,
and a void* constructor that skips populating, for parity with the
existing per-signal options classes.
OtlpGrpcExporterOptions / OtlpGrpcMetricExporterOptions /
OtlpGrpcLogRecordExporterOptions each gained a constructor taking a
const 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.
Build files updated for the new source file and one new Bazel dep
(//sdk/src/common:env_variables); no new library targets, no change
to the existing dependency graph shape.
New unit test otlp_grpc_client_options_test.cc covering: default
env-var population, the void* no-defaults path (including the
derived classes' void* constructors), fixed-field copying from a
shared 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-format applied.

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>

linux-foundation-easycla · 2026-07-15T09:51:12Z

The committers listed above are authorized under a signed CLA.

✅ login: ML-dev-crypto / name: Ansh Rai (4998b72)

codecov · 2026-07-15T18:12:54Z

Codecov Report

❌ Patch coverage is 97.26027% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.15%. Comparing base (4e20f69) to head (4998b72).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...de/opentelemetry/exporters/otlp/otlp_environment.h	95.88%	4 Missing ⚠️

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

Files with missing lines	Coverage Δ
...elemetry/exporters/otlp/otlp_grpc_client_options.h	`100.00% <ø> (ø)`
...emetry/exporters/otlp/otlp_grpc_exporter_options.h	`100.00% <ø> (ø)`
...rters/otlp/otlp_grpc_log_record_exporter_options.h	`100.00% <ø> (ø)`
...exporters/otlp/otlp_grpc_metric_exporter_options.h	`100.00% <ø> (ø)`
exporters/otlp/src/otlp_grpc_client_options.cc	`100.00% <100.00%> (ø)`
exporters/otlp/src/otlp_grpc_exporter_options.cc	`100.00% <100.00%> (+19.24%)`	⬆️
.../otlp/src/otlp_grpc_log_record_exporter_options.cc	`100.00% <100.00%> (+19.24%)`	⬆️
...ters/otlp/src/otlp_grpc_metric_exporter_options.cc	`100.00% <100.00%> (+21.43%)`	⬆️
...de/opentelemetry/exporters/otlp/otlp_environment.h	`96.30% <95.88%> (-3.70%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Ansh Rai <anshrai331@gmail.com>

owent · 2026-07-16T13:43:02Z

 {
-  virtual ~OtlpGrpcClientOptions()                                = default;
-  OtlpGrpcClientOptions()                                         = default;
+  virtual ~OtlpGrpcClientOptions();


In my understanding, if we implement public struct or function in .cc files. We need to use OPENTELEMETRY_EXPORT to export them. Or the symbols will be hidden and cause link error in some situation.

You're right, I missed that. I'll add OPENTELEMETRY_EXPORT.

owent · 2026-07-16T13:55:30Z

+namespace otlp
+{
+
+OtlpGrpcClientOptions::OtlpGrpcClientOptions()


Should members be initialized in a member initializer of the constructor to avoid cppcoreguidelines-prefer-member-initializer of clang-tidy?

Good point. I'll switch these to use a member initializer list to satisfy cppcoreguidelines-prefer-member-initializer.

owent · 2026-07-16T14:04:54Z

+inline std::string GetOtlpDefaultGrpcClientSslClientKeyPath()
+{
+  std::string value;
+  opentelemetry::sdk::common::GetStringEnvironmentVariable("OTEL_EXPORTER_OTLP_CLIENT_KEY", value);


I'd suggest moving these environment variable definitions to a shared location so both this file and otlp_environment.cc can reference the same constants. This will make maintenance easier going forward.

Agreed. The generic OTLP environment variable names are duplicated now, so I'll move them to a shared location that both the header and implementation can reference.

owent · 2026-07-16T14:06:33Z

+
+  std::unordered_set<std::string> remove_cache;
+
+  while (tokenizer.next(header_valid, header_key, header_value))


Why not export and use DumpOtlpHeaders here?

Makes sense. I duplicated the parsing logic because DumpOtlpHeaders isn't currently reusable, but I agree it's better to avoid the duplication. I'll refactor this to reuse the existing implementation.

owent · 2026-07-16T14:19:31Z

+ * environment variables and fall back to the same spec defaults used above.
+ */
+
+inline std::string GetOtlpDefaultGrpcClientEndpoint()


We should move these implementations into otlp_environment.cc to reduce symbol size.
Also, the inline keyword is merely a hint to the compiler, not a mandatory directive. Different compilers may make different inlining decisions under different optimization flags, which can lead to inconsistent symbol generation.
For example, in Release builds the compiler might fully inline these functions and omit external symbols. Later, when a Debug build links against this library, the linker may not resolve those symbols, resulting in undefined references at link time.

Good point. I'll move these helper implementations out of the header and keep them alongside the existing environment accessors while making sure the dependency structure still works.

owent · 2026-07-16T14:21:26Z

Should we use OtlpGrpcClientOptions(nullptr) here and in constructor of metrics and logs options to avoid unnecessary environment parsing?

Agreed.I'll update this to reuse the base constructor and only override the signal-specific fields.

owent · 2026-07-16T14:26:47Z

+  metadata = GetOtlpDefaultTracesHeaders();
+
+#ifdef ENABLE_ASYNC_EXPORT
+  max_concurrent_requests = 64;


Why reset max_concurrent_requests here?

Thanks for catching that,The intention was to preserve the shared client's configured value, so I'll update this constructor to copy max_concurrent_requests instead of resetting it.

owent · 2026-07-16T14:27:04Z

+  metadata = GetOtlpDefaultLogsHeaders();
+
+#ifdef ENABLE_ASYNC_EXPORT
+  max_concurrent_requests = 64;


Just as above, why reset max_concurrent_requests here?

Thanks! I'll make the same change here as well.

ML-dev-crypto · 2026-07-16T17:14:31Z

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 max_concurrent_requests value. I'll push everything together.

Signed-off-by: Ansh Rai <anshrai331@gmail.com>

ML-dev-crypto requested a review from a team as a code owner July 15, 2026 09:51

Add missing trailing newline (fixes -Wnewline-eof in maintainer mode)

a6cb608

Signed-off-by: Ansh Rai <anshrai331@gmail.com>

owent reviewed Jul 16, 2026

View reviewed changes

Address review feedback for OtlpGrpcClientOption

3afcbb1

Signed-off-by: Ansh Rai <anshrai331@gmail.com>


		std::unordered_set<std::string> remove_cache;

		while (tokenizer.next(header_valid, header_key, header_value))

Uh oh!

Conversation

ML-dev-crypto commented Jul 15, 2026

Uh oh!

linux-foundation-easycla Bot commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jul 15, 2026

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ML-dev-crypto Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owent Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ML-dev-crypto commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

linux-foundation-easycla Bot commented Jul 15, 2026 •

edited

Loading

ML-dev-crypto Jul 16, 2026 •

edited

Loading

owent Jul 16, 2026 •

edited

Loading

ML-dev-crypto commented Jul 16, 2026 •

edited

Loading