-
Notifications
You must be signed in to change notification settings - Fork 715
opentelemetry-exporter-otlp-proto-grpc: set grpc user agent properly #4658
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
base: main
Are you sure you want to change the base?
Conversation
0553378
to
bed8002
Compare
...pentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/exporter.py
Show resolved
Hide resolved
@@ -60,6 +60,7 @@ def __init__( | |||
] = None, | |||
timeout: Optional[float] = None, | |||
compression: Optional[Compression] = None, | |||
channel_options: Optional[TypingSequence[Tuple[str, str]]] = None, |
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.
Seems fine to me. I guess the alternative is allowing to pass a gRPC channel directly, but then the user has to configure all the other options.
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.
I don't expect enduser to play with this, I've opened this for being able to pass exporter params from sdk configuration #4659
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.
Not blocking just a callout
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.
Thinking about http though maybe it makes more sense to use a shared way for providing the user agent? OTOH it may useful to tune some other grpc options
@@ -73,4 +73,9 @@ | |||
from .version import __version__ | |||
|
|||
_USER_AGENT_HEADER_VALUE = "OTel-OTLP-Exporter-Python/" + __version__ | |||
# these will be sent as grpc metadata | |||
_OTLP_GRPC_HEADERS = [("user-agent", _USER_AGENT_HEADER_VALUE)] |
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.
I think the user-agent
entry here is maybe a no-op and could be removed. IIRC gRPC C++ unconditionally overwrites this metadata key which is why we need to pass it through the channel options.
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.
Right, that's the case, removed.
It looks like metadata is ignored and instead we should set the grpc.primary_user_agent channel option instead. User-agent will change from: grpc-python/1.71.0 grpc-c/46.0.0 (linux; chttp2) to: OTel-OTLP-Exporter-Python/1.34.1 grpc-python/1.71.0 grpc-c/46.0.0 (linux; chttp2)
649b5e8
to
3f4f7bd
Compare
Description
It looks like metadata is not used for setting http User-agent header when using the grpc exporter and instead we should set the
grpc.primary_user_agent
channel option instead. User-agent will change from:grpc-python/1.71.0 grpc-c/46.0.0 (linux; chttp2)
to:
OTel-OTLP-Exporter-Python/1.34.1 grpc-python/1.71.0 grpc-c/46.0.0 (linux; chttp2)
Fixes #4657
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Contrib Repo Change?
Checklist: