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 10 commits into
Conversation
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4249 +/- ##
==========================================
+ Coverage 82.58% 82.67% +0.09%
==========================================
Files 511 512 +1
Lines 20039 20118 +79
==========================================
+ Hits 16548 16631 +83
+ Misses 3491 3487 -4
🚀 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 |
| { | ||
| 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
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 |
Could you please try to add |
Signed-off-by: Ansh Rai <anshrai331@gmail.com>
|
Thanks for the suggestion! I've updated the PR by adding |
| metadata = GetOtlpDefaultTracesHeaders(); | ||
|
|
||
| #ifdef ENABLE_ASYNC_EXPORT | ||
| max_concurrent_requests = client_options.max_concurrent_requests; |
There was a problem hiding this comment.
One shared-client edge case: the gRPC client only uses this when its async data is first created. With one client shared by trace/metric/log exporters, the first exporter wins and later per-signal values are ignored.
There was a problem hiding this comment.
Thanks for pointing this out. I agree this is a limitation of the shared async client rather than the option-copying logic in this PR. I'd prefer to keep this PR focused on the current fixes, but I'm happy to address it in a follow-up PR .
| OtlpGrpcExporterOptions::OtlpGrpcExporterOptions(const OtlpGrpcClientOptions &client_options) | ||
| : OtlpGrpcClientOptions(client_options) | ||
| { | ||
| timeout = GetOtlpDefaultTracesTimeout(); |
There was a problem hiding this comment.
I think this re-reads env vars instead of falling back to client_options.timeout. If the shared client timeout was set programmatically and no trace-specific env var exists, this drops that value and goes back to the default.
There was a problem hiding this comment.
Thanks for catching this! I've updated the implementation to preserve client_options.timeout unless a signal-specific timeout override is present, and added a regression test covering the programmatic shared-client timeout case.
Signed-off-by: Ansh Rai <anshrai331@gmail.com>
| { | ||
| timeout = signal_timeout; | ||
| } | ||
| metadata = GetOtlpDefaultTracesHeaders(); |
There was a problem hiding this comment.
The base-class copy on line 52 already preserves client_options.metadata, but line 59 immediately discards it and rebuilds the map only from OTLP header environment variables.
With no header environment variables:
OtlpGrpcClientOptions client(nullptr);
client.metadata.emplace("authorization", "credential");
OtlpGrpcExporterOptions trace(client);trace.metadata becomes empty, so programmatic authentication is silently lost.
Could we preserve the copied map and overlay only OTEL_EXPORTER_OTLP_TRACES_HEADERS onto it, with signal-specific values replacing same-name client values? That matches the existing generic-then-signal header precedence. Please apply the same fix to metrics and logs, and add regression tests for preservation without a signal variable plus merge/override behavior when one is present.
There was a problem hiding this comment.
Thanks for pointing this out. I’ll preserve the metadata copied from client_options and overlay the signal-specific headers on top, with signal-specific values taking precedence for the same key. I’ll apply the same fix to traces, metrics, and logs and add the regression tests.
|
About the undefined reference symbols. The symbols should be declared as visibility=defaut on Unix like system and
if ("${OPENTELEMETRY_OTLP_TARGETS_LIB_TYPE}" STREQUAL "STATIC" OR NOT BUILD_SHARED_LIBS)
project_build_tools_set_shared_library_declaration(OPENTELEMETRY_OTLP_RECORDABLE_API
opentelemetry_otlp_recordable)
else()
project_build_tools_set_static_library_declaration(OPENTELEMETRY_OTLP_RECORDABLE_API
opentelemetry_otlp_recordable)
endif()
if ("${OPENTELEMETRY_OTLP_GRPC_CLIENT_LIB_TYPE}" STREQUAL "STATIC" OR NOT BUILD_SHARED_LIBS)
project_build_tools_set_shared_library_declaration(OPENTELEMETRY_OTLP_GRPC_CLIENT_API
opentelemetry_exporter_otlp_grpc_client)
else()
project_build_tools_set_static_library_declaration(OPENTELEMETRY_OTLP_GRPC_CLIENT_API
opentelemetry_exporter_otlp_grpc_client)
endif()
In #ifndef OPENTELEMETRY_OTLP_RECORDABLE_API
# if defined(__clang__)
# define OPENTELEMETRY_OTLP_RECORDABLE_API __attribute__((visibility("default")))
# elif defined(__GNUC__)
# define OPENTELEMETRY_OTLP_RECORDABLE_API __attribute__((visibility("default")))
# else
# define OPENTELEMETRY_OTLP_RECORDABLE_API
# endif
#endifAnd in #ifndef OPENTELEMETRY_OTLP_GRPC_CLIENT_API
# if defined(__clang__)
# define OPENTELEMETRY_OTLP_GRPC_CLIENT_API __attribute__((visibility("default")))
# elif defined(__GNUC__)
# define OPENTELEMETRY_OTLP_GRPC_CLIENT_API __attribute__((visibility("default")))
# else
# define OPENTELEMETRY_OTLP_GRPC_CLIENT_API
# endif
#endif
|
|
@owent Should the project_build_tools_set_*_library_declaration() calls target opentelemetry_otlp_common instead? And should I keep OPENTELEMETRY_OTLP_RECORDABLE_API as the macro name, or rename it to OPENTELEMETRY_OTLP_COMMON_API to match the target? |
The new target (opentelemetry_otlp_common) is used and I think the macro name should be
|
dbarker
left a comment
dbarker
left a comment
There was a problem hiding this comment.
Hi @ML-dev-crypto , Could you please resolve the CI issues? Once passing I'll approve.
Please see notes below on the fixes needed.
| namespace otlp | ||
| { | ||
|
|
||
| OtlpGrpcClientOptions::OtlpGrpcClientOptions() |
There was a problem hiding this comment.
clang-tidy warnings from this file:
| Line | Check | Message |
|---|---|---|
| 22 | cppcoreguidelines-use-default-member-init,modernize-use-default-member-init | member initializer for 'max_threads' is redundant |
| 42 | cppcoreguidelines-prefer-member-initializer | 'use_ssl_credentials' should be initialized in a member initializer of the constructor |
| 43 | cppcoreguidelines-prefer-member-initializer | 'max_threads' should be initialized in a member initializer of the constructor |
| 46 | cppcoreguidelines-prefer-member-initializer | 'max_concurrent_requests' should be initialized in a member initializer of the constructor |
|
|
||
| namespace | ||
| { | ||
| class ScopedEnvVar |
There was a problem hiding this comment.
clang-tidy warning for the special member functions
`cppcoreguidelines-special-member-functions` class 'ScopedEnvVar' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator|
|
||
| OtlpGrpcMetricExporterOptions::OtlpGrpcMetricExporterOptions( | ||
| const OtlpGrpcClientOptions &client_options) | ||
| : OtlpGrpcClientOptions(client_options), |
There was a problem hiding this comment.
clang-tidy warnings from this file:
| Line | Check | Message |
|---|---|---|
| 50 | cppcoreguidelines-use-default-member-init,modernize-use-default-member-init | member initializer for 'aggregation_temporality' is redundant |
| 63 | cppcoreguidelines-use-default-member-init,modernize-use-default-member-init | member initializer for 'aggregation_temporality' is redundant |
| #include <chrono> | ||
| #include <string> | ||
|
|
||
| #include "opentelemetry/exporters/otlp/otlp_environment.h" |
There was a problem hiding this comment.
IWYU warning for this file.
Warning: include-what-you-use reported diagnostics:
/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/exporters/otlp/src/otlp_grpc_log_record_exporter_options.cc should add these lines:
#include "opentelemetry/exporters/otlp/otlp_grpc_client_options.h"
/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/exporters/otlp/src/otlp_grpc_log_record_exporter_options.cc should remove these lines:
The full include-list for /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/exporters/otlp/src/otlp_grpc_log_record_exporter_options.cc:
#include <chrono>
#include <string>
#include "opentelemetry/exporters/otlp/otlp_environment.h"
#include "opentelemetry/exporters/otlp/otlp_grpc_client_options.h"
#include "opentelemetry/exporters/otlp/otlp_grpc_log_record_exporter_options.h"
#include "opentelemetry/version.h"
---
|
|
||
| #include "opentelemetry/exporters/otlp/otlp_environment.h" | ||
| #include "opentelemetry/exporters/otlp/otlp_grpc_metric_exporter_options.h" | ||
| #include "opentelemetry/version.h" |
There was a problem hiding this comment.
IWYU warning for this file:
Warning: include-what-you-use reported diagnostics:
/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/exporters/otlp/src/otlp_grpc_metric_exporter_options.cc should add these lines:
#include "opentelemetry/exporters/otlp/otlp_grpc_client_options.h"
#include "opentelemetry/exporters/otlp/otlp_preferred_temporality.h"
/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/exporters/otlp/src/otlp_grpc_metric_exporter_options.cc should remove these lines:
The full include-list for /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/exporters/otlp/src/otlp_grpc_metric_exporter_options.cc:
#include <chrono>
#include <string>
#include "opentelemetry/exporters/otlp/otlp_environment.h"
#include "opentelemetry/exporters/otlp/otlp_grpc_client_options.h"
#include "opentelemetry/exporters/otlp/otlp_grpc_metric_exporter_options.h"
#include "opentelemetry/exporters/otlp/otlp_preferred_temporality.h"
#include "opentelemetry/version.h"
---| #include "opentelemetry/exporters/otlp/otlp_grpc_client_options.h" | ||
| #include "opentelemetry/exporters/otlp/otlp_grpc_exporter_options.h" | ||
| #include "opentelemetry/exporters/otlp/otlp_grpc_log_record_exporter_options.h" | ||
| #include "opentelemetry/exporters/otlp/otlp_grpc_metric_exporter_options.h" |
There was a problem hiding this comment.
IWYU warning for this file:
Warning: include-what-you-use reported diagnostics:
/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/exporters/otlp/test/otlp_grpc_client_options_test.cc should add these lines:
#include "opentelemetry/version.h"
/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/exporters/otlp/test/otlp_grpc_client_options_test.cc should remove these lines:
The full include-list for /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/exporters/otlp/test/otlp_grpc_client_options_test.cc:
#include <gtest/gtest.h>
#include <chrono>
#include <cstdlib>
#include <string>
#include "opentelemetry/exporters/otlp/otlp_grpc_client_options.h"
#include "opentelemetry/exporters/otlp/otlp_grpc_exporter_options.h"
#include "opentelemetry/exporters/otlp/otlp_grpc_log_record_exporter_options.h"
#include "opentelemetry/exporters/otlp/otlp_grpc_metric_exporter_options.h"
#include "opentelemetry/version.h"
---|
|
||
| #include <chrono> | ||
| #include <memory> | ||
| #include <string> |
There was a problem hiding this comment.
IWYU warning for this file:
Warning: include-what-you-use reported diagnostics:
/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_grpc_client_options.h should add these lines:
#include <cstddef> // for size_t
#include <cstdint> // for uint32_t
/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_grpc_client_options.h should remove these lines:
- #include <memory> // lines 10-10
- namespace grpc { class ChannelCredentials; } // lines 15-15
The full include-list for /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_grpc_client_options.h:
#include <chrono> // for duration
#include <cstddef> // for size_t
#include <cstdint> // for uint32_t
#include <string> // for basic_string
#include "opentelemetry/exporters/otlp/otlp_environment.h" // for OtlpHeaders
#include "opentelemetry/version.h" // for OPENTELEM...
namespace grpc { class ChannelArguments; } // lines 16-16
---|
|
||
| #include "opentelemetry/exporters/otlp/otlp_environment.h" | ||
| #include "opentelemetry/exporters/otlp/otlp_grpc_exporter_options.h" | ||
| #include "opentelemetry/version.h" |
There was a problem hiding this comment.
IWYU warning for this file:
Warning: include-what-you-use reported diagnostics:
/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/exporters/otlp/src/otlp_grpc_exporter_options.cc should add these lines:
#include "opentelemetry/exporters/otlp/otlp_grpc_client_options.h"
/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/exporters/otlp/src/otlp_grpc_exporter_options.cc should remove these lines:
The full include-list for /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/exporters/otlp/src/otlp_grpc_exporter_options.cc:
#include <chrono>
#include <string>
#include "opentelemetry/exporters/otlp/otlp_environment.h"
#include "opentelemetry/exporters/otlp/otlp_grpc_client_options.h"
#include "opentelemetry/exporters/otlp/otlp_grpc_exporter_options.h"
#include "opentelemetry/version.h"
---
| { | ||
|
|
||
| struct OtlpGrpcClientOptions | ||
| struct OPENTELEMETRY_EXPORT OtlpGrpcClientOptions |
There was a problem hiding this comment.
RE: #4249 (comment)
@owent since this class now uses the same export declaration as the otlp exporter options classes that derive from it, would it be reasonable to defer any changes to those definitions to a follow up PR?
There was a problem hiding this comment.
The export/import declaration should follow the target. What I mean is:
- Public functions in otlp_environment.cc should be declared as OPENTELEMETRY_OTLP_COMMON_API, since that file is built into the opentelemetry_otlp_common target.
- Public functions in otlp_grpc_client.cc should be declared as OPENTELEMETRY_OTLP_GRPC_CLIENT_API.
As for OtlpGrpcClientOptions, it's fine to leave it without an export declaration — it's a header-only class with no source file. Every target that references it will compile its own copy. This only adds a bit of linker load; there's no visibility issue.
There was a problem hiding this comment.
The export/import declaration should follow the target.
That makes sense to me. Since this would be a new policy for the otlp exporter folder (and likely needed for the SDK classes as well), is it reasonable to apply the new policy in follow-up PRs?
Properly setting the export/import declarations (for all platforms and on the intended public facing classes) is something we need to address broadly throughout the project.
As for OtlpGrpcClientOptions, it's fine to leave it without an export declaration — it's a header-only class with no source file.
This PR adds the .cc file for OtlpGrpcClientOptions and may have prompted this discussion.
There was a problem hiding this comment.
That makes sense to me. Since this would be a new policy for the otlp exporter folder (and likely needed for the SDK classes as well), is it reasonable to apply the new policy in follow-up PRs?
My suggestion is to apply it to all components. If we export symbols this way consistently across all components, ext/src/dll/input.src would no longer be needed, and we could keep a uniform symbol-exporting rule across all platforms, compilers, and optimization settings — by setting the default visibility to hidden on Unix-like systems. It would also reduce linker load and speed up linking. (For example, some compiler versions may inline a symbol in Release builds but keep it "imported" in Debug builds, which leads to link errors. This issue has occurred with certain versions of protobuf.)
This PR adds the .cc file for OtlpGrpcClientOptions and may have prompted this discussion.
Sorry, I missed that. Then it should use something like OPENTELEMETRY_OTLP_GRPC_CLIENT_API to export its symbols.
There was a problem hiding this comment.
Applying the symbol visibility attributes to all components sounds like a nice improvement.
The grpc client class doesn't have symbol visibility attributes now so the change must be broader than this PR should take on.
Let's merge this once CI is passing and address the visibility attributes in a new PR that fixes the grpc client as well.
5 participants
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.