feat: add user agent option for HTTP and GRPC exporters#5928
feat: add user agent option for HTTP and GRPC exporters#5928david-luna merged 42 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5928 +/- ##
=======================================
Coverage 95.24% 95.24%
=======================================
Files 315 315
Lines 8602 8604 +2
Branches 1795 1798 +3
=======================================
+ Hits 8193 8195 +2
Misses 409 409
🚀 New features to boost your workflow:
|
…pentelemetry-js into exporters-user-agent-option
| super( | ||
| createOtlpHttpExportDelegate( | ||
| convertLegacyHttpOptions(config, 'LOGS', 'v1/logs', { | ||
| 'User-Agent': `OTel-OTLP-Exporter-JavaScript/${VERSION}`, |
There was a problem hiding this comment.
note for reviewer: the default value is moved to the exporter-base package. Users can pass now a secondary user agent in the config in the constructor.
new OTLPLogExporter({ userAgent: 'Custom-UA/1.2.3' });There was a problem hiding this comment.
the default value was here, since I was not sure if we would ever unpin the dependency. By using VERSION from the base package, the version might differ from the actual package version.
(should not make a difference realistically, as we'd likely always have it at ^whatever-the-concrete-exporter-version-is and almost all the logic is in the base package.)
There was a problem hiding this comment.
My reasoning for this change was that the factory functions come from the base package. Hence if there is any change in these factory functions the version bump will happen in there. This package will have a version bump too if updating the dependency from base but the semantics might be different.
There was a problem hiding this comment.
Sounds good 👍
Just wanted to elaborate on my initial decision to put it here. I'm fine with it being in the base :)
| signalResourcePath: string, | ||
| requiredHeaders: Record<string, string> | ||
| ): OtlpNodeHttpConfiguration { | ||
| ): Required<OtlpNodeHttpConfiguration> { |
There was a problem hiding this comment.
note for reviewer: I guess OtlpNodeHttpConfiguration is going to be the replacement for the legacy options which are used at the moment. So I kept the userAgent optional for that interface which is the one exposed to the user. The property gets set when calling mergeOtlpNodeHttpConfigurationWithDefaults so that's the reason to use the Required utility type.
There was a problem hiding this comment.
Yep, it was a blunder on my part that I did not use the Required type from the get-go.
+1 for that change.
|
|
||
| export function createOtlpHttpExportDelegate<Internal, Response>( | ||
| options: OtlpNodeHttpConfiguration, | ||
| options: Required<OtlpNodeHttpConfiguration>, |
There was a problem hiding this comment.
note for reviewer: I have a doubt here. Is this function meant to be called by users? If so I should mark this PR as a breaking change
There was a problem hiding this comment.
Please correct me if I'm wrong. The HttpRequestParameters type could make the added userAgent property optional, right? Then there wouldn't be a need to use the Required<...> utility.
I could understand if the desire is to avoid optional params for (mostly) internal types.
There was a problem hiding this comment.
At the time of doing the changes it seemed a good option to resolve the user-agent value when merging option values from different sources (user, environment & default).
Making it optional here it means each transport implementation is responsible to set the default agent and prepend the userAgent option if passed. But now I think is not a big deal.
I'll push a new commit with the required changes,
| }, | ||
| new http.Agent(), | ||
| Buffer.from([1, 2, 3]), | ||
| // TODO: the `onDone` callback is called twice because there are two error handlers |
There was a problem hiding this comment.
note for reviewer: this is a question for the code owner. Is the callback meat to be called twice upon error?
There was a problem hiding this comment.
it's not supposed to, no.
Can be tackled separate issue:
We should change it to be promise-based like the transport to simplify it. That would also solve the problem here as only the first resolve/reject counts.
There was a problem hiding this comment.
I've created #5990. We can triage it in the next SIG
experimental/packages/otlp-exporter-base/src/configuration/legacy-node-configuration.ts
Outdated
Show resolved
Hide resolved
experimental/packages/otlp-exporter-base/src/configuration/legacy-node-configuration.ts
Outdated
Show resolved
Hide resolved
experimental/packages/otlp-exporter-base/src/configuration/otlp-node-http-configuration.ts
Outdated
Show resolved
Hide resolved
|
|
||
| export function createOtlpHttpExportDelegate<Internal, Response>( | ||
| options: OtlpNodeHttpConfiguration, | ||
| options: Required<OtlpNodeHttpConfiguration>, |
There was a problem hiding this comment.
Please correct me if I'm wrong. The HttpRequestParameters type could make the added userAgent property optional, right? Then there wouldn't be a need to use the Required<...> utility.
I could understand if the desire is to avoid optional params for (mostly) internal types.
…pentelemetry-js into exporters-user-agent-option
| sendUserAgent = opts.headers['User-Agent']; | ||
| return http.request(opts, cb).destroy(); | ||
| }; | ||
| let sendUserAgent: string; |
There was a problem hiding this comment.
nit: perhaps call it sentUserAgent, because IIUC this is about recording the user-agent that was sent in the client request.
experimental/packages/otlp-exporter-base/test/node/http-transport-utils.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Trent Mick <trentm@gmail.com>
…ort-utils.test.ts Co-authored-by: Trent Mick <trentm@gmail.com>
| super( | ||
| createOtlpHttpExportDelegate( | ||
| convertLegacyHttpOptions(config, 'LOGS', 'v1/logs', { | ||
| 'User-Agent': `OTel-OTLP-Exporter-JavaScript/${VERSION}`, |
There was a problem hiding this comment.
the default value was here, since I was not sure if we would ever unpin the dependency. By using VERSION from the base package, the version might differ from the actual package version.
(should not make a difference realistically, as we'd likely always have it at ^whatever-the-concrete-exporter-version-is and almost all the logic is in the base package.)
| signalResourcePath: string, | ||
| requiredHeaders: Record<string, string> | ||
| ): OtlpNodeHttpConfiguration { | ||
| ): Required<OtlpNodeHttpConfiguration> { |
There was a problem hiding this comment.
Yep, it was a blunder on my part that I did not use the Required type from the get-go.
+1 for that change.
| }, | ||
| new http.Agent(), | ||
| Buffer.from([1, 2, 3]), | ||
| // TODO: the `onDone` callback is called twice because there are two error handlers |
There was a problem hiding this comment.
it's not supposed to, no.
Can be tackled separate issue:
We should change it to be promise-based like the transport to simplify it. That would also solve the problem here as only the first resolve/reject counts.
pichlermarc
left a comment
There was a problem hiding this comment.
looks good, just a few nits 🙂
| super( | ||
| createOtlpHttpExportDelegate( | ||
| convertLegacyHttpOptions(config, 'LOGS', 'v1/logs', { | ||
| 'User-Agent': `OTel-OTLP-Exporter-JavaScript/${VERSION}`, |
There was a problem hiding this comment.
Sounds good 👍
Just wanted to elaborate on my initial decision to put it here. I'm fine with it being in the base :)
experimental/packages/otlp-exporter-base/test/node/http-transport-utils.test.ts
Outdated
Show resolved
Hide resolved
experimental/packages/otlp-grpc-exporter-base/test/grpc-exporter-transport.test.ts
Outdated
Show resolved
Hide resolved
…ort-utils.test.ts Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
…er-transport.test.ts Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
Which problem is this PR solving?
Closes: #5860
Fixes: #5867
Description of the changes
Summary
At its core the PR is adding a new optional property in the legacy and new new configuration options for HTTP and GRPC. It also unifies how both configurations are resolved regarding the user-agent header/metadata.
Details
Current implementations where using
headersandmetadatafunctions to pass the user agent. I preferred to have a specific option for that.The default user agent for HTTP has been moved to the base exporter packages as a default config option. Then the merge mechanism takes care (concats) if the user has provided another value. This way we can remove the default value of user agent from all http exporters. The resolved user agent will be passed to transport and this will be used for exports, ignoring any possible user agent value in the
headersfunction. This approach is similar to what GRPC is doing and I prefer it since IMHO there should be no differences in exporters.For GRPC we do the same to resolve the user agent but we use it in the GRPC service constructor. Turns out adding into metadata was not enough and the internals of the library just ignore that entry. This contributes to fix #5867
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
Checklist: