-
Notifications
You must be signed in to change notification settings - Fork 486
[SDK] Add credentials option to OTLP gRPC client (#3402) #3403
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.
|
8f764f4
to
78d8286
Compare
Am I correct that the ABI Policy says this change is exempt because it is to the SDK, rather then API? |
exporters/otlp/include/opentelemetry/exporters/otlp/otlp_grpc_client_options.h
Show resolved
Hide resolved
That's correct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. We need one more approval to get this through.
exporters/otlp/include/opentelemetry/exporters/otlp/otlp_grpc_client_options.h
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3403 +/- ##
=======================================
Coverage 89.92% 89.92%
=======================================
Files 219 219
Lines 7041 7041
=======================================
Hits 6331 6331
Misses 710 710 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please clarify:
-
How does this proposal fits with OTEP open-telemetry/opentelemetry-specification#4500
-
This seem to cover the programmatic configuration only. I assume configuration of custom credentials using environment variables is out of scope at this point, please confirm.
-
How, probably not yet but in the long term, would custom credential work with declarative configuration (see https://github.com/open-telemetry/opentelemetry-configuration/blob/f27ee13bbc7ee8156954aea79b1d8bfb0312b7cc/schema/common.json#L82), where the Grpc configuration is expressed in yaml ?
I support this change, but we need to make sure it fits the overall picture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR.
To be able to merge this to the main branch, the following changes are necessary.
Please try a forward declaration instead of an include of grpc++ headers.
User code creating a grpc exporter may not have the grpc++ headers installed,
so this change can be breaking, especially for people who will not be using this feature.
This will also resolve the CI issues.
Given that this feature is not stable in the specs, it needs a feature flag.
Suggested naming:
- CMake option
WITH_OTLP_GRPC_CREDENTIAL_PREVIEW
- define
ENABLE_OTLP_GRPC_CREDENTIAL_PREVIEW
See the existing option WITH_OTLP_GRPC_SSL_MTLS_PREVIEW
for how to implement this.
280a3fb
to
b891aaa
Compare
Ok, I’ve had some time to work on this again. I’ve addressed the failing tests and comments above with a forward declaration. I’ve also added a warning log if both It is behind a feature flag supported by both CMake How this related to OTEP, environment variables and other types of declarative configuration is an open question. I see this new |
b891aaa
to
5b25596
Compare
This allows passing custom `ChannelCredentials` when creating gRPC channels, to support authentication protocols that require short-lived tokens.
5b25596
to
383a951
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the feature.
Adds a
credentials
option toOtlpGrpcClientOptions
that allows specifying aChannelCredentials
object, rather than it being created inOtlpGrpcClient::MakeChannel
with eithergrpc::SslCredentials()
orgrpc::InsecureChannelCredentials()
.It allows using a custom
MetadataCredentialsPlugin
subclass to support arbitrary authentication methods, or an existing implementation likeGoogleDefaultCredentials()
for use with the GCP OTLP API.This is similar to the solution used by OpenTelemetry Python – its OTLP exporters have a
credentials
parameter.Fixes #3402
Changes
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes