feat: informative user-agent for otlp exporters #3970

vreynolds · 2021-09-03T19:41:42Z

Description: This adds a default user-agent header to outgoing requests by the OTLP/gRPC and OTLP/HTTP exporters. The default user-agent contains build information about the collector.

Behavior details:

Currently, it's possible to add a user-agent header to the HTTP exporter via the headers config. This change preserves this behavior, and only sets the collector user-agent, if one is not present in config headers.
Currently, it's not possible to do the above with a gRPC exporter. This change does not affect that, and only updates the existing gRPC client user-agent.

Link to tracking Issue: Fixes #2209

Testing: Updated/added to existing unit tests. Built a local collector and verified outgoing user-agent headers in both exporters.

Documentation: N/A?

- add collector build info to otlp/otlphttp exporter headers

config/confighttp/confighttp.go

config/configgrpc/configgrpc.go

exporter/exporterhelper/common.go

- move user-agent additions to the exporters - add OS and ARCH to user-agent

codeboten

👍 this is a good feature to add. Just one question regarding the following in the description:

Currently, it's possible to add a user-agent header to the HTTP exporter via the headers config. This change preserves this behavior, and only sets the collector user-agent, if one is not present in config headers.
Currently, it's not possible to do the above with a gRPC exporter. This change does not affect that, and only updates the existing gRPC client user-agent.

My understanding is that this is currently possible for the grpc exporter via the headers configuration and and not for the http exporter, but the comment in the description makes me think it's the other way around. Can you confirm this?

exporter/otlpexporter/otlp.go

vreynolds · 2021-09-09T19:22:22Z

My understanding is that this is currently possible for the grpc exporter via the headers configuration and and not for the http exporter, but the comment in the description makes me think it's the other way around. Can you confirm this?

Confirmed in local testing: http exporter allows setting user-agent via headers config, grpc does not

For the http client, config headers are set as request headers, which is how a user-agent gets added canonically.
For the grpc client, config headers are set as metadata, whereas a user-agent needs to be set as a dial option.

codeboten · 2021-09-09T19:33:06Z

My understanding is that this is currently possible for the grpc exporter via the headers configuration and and not for the http exporter, but the comment in the description makes me think it's the other way around. Can you confirm this?

Confirmed in local testing: http exporter allows setting user-agent via headers config, grpc does not
* For the `http` client, config headers are set as request headers, which is how a user-agent gets added canonically.

* For the `grpc` client, config headers are set as metadata, whereas a user-agent needs to be set as a dial option.

Thanks for clarifying 👍

config/confighttp/confighttp.go

exporter/otlphttpexporter/otlp.go

github-actions · 2021-09-21T03:18:10Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

codeboten

PR looks good, please resolve the conflict.

vreynolds · 2021-09-21T18:50:15Z

Merged upstream, and a couple small updates adding a doc comment and parenthesis around (OS/ARCH)

github-actions · 2021-09-29T03:15:47Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

vreynolds · 2021-10-04T15:07:55Z

Didn't get a chance to review the feedback last week, but will do that this week 🙏

codecov · 2021-10-06T18:23:48Z

Codecov Report

Merging #3970 (a8bd4ee) into main (96a882a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #3970   +/-   ##
=======================================
  Coverage   90.77%   90.78%           
=======================================
  Files         179      179           
  Lines       10412    10423   +11     
=======================================
+ Hits         9452     9463   +11     
  Misses        743      743           
  Partials      217      217

Impacted Files	Coverage Δ
exporter/otlpexporter/factory.go	`91.30% <100.00%> (ø)`
exporter/otlpexporter/otlp.go	`74.41% <100.00%> (+1.57%)`	⬆️
exporter/otlphttpexporter/factory.go	`87.36% <100.00%> (ø)`
exporter/otlphttpexporter/otlp.go	`82.92% <100.00%> (+0.87%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96a882a...a8bd4ee. Read the comment docs.

vreynolds · 2021-11-03T17:14:12Z

@bogdandrutu yes, I plan on updating this today -- are you OK with a custom roundtripper that clones the request and updates the header?

bogdandrutu

few more comments about limiting the public API, and avoid implementing roundtripper if we don't intend that to be used by others.

bogdandrutu · 2021-11-09T17:35:10Z

exporter/exporterhelper/common.go

@@ -232,3 +234,8 @@ func (ts *timeoutSender) send(req request) error {
 	}
 	return req.export(ctx)
 }
+
+// DefaultUserAgent User-Agent value used by exporters
+func DefaultUserAgent(info component.BuildInfo) string {


let's move this to otlpexporter for the moment or an internal package. Prefer to limit the public API.

✅ Moved to each exporter f5bddaf

bogdandrutu · 2021-11-09T17:36:17Z

exporter/otlphttpexporter/otlp.go

@@ -213,3 +218,32 @@ func readResponse(resp *http.Response) *status.Status {

 	return respStatus
 }
+
+type userAgentRoundTripper struct {


Do we really need a roundtripper? Since this is private, we should just simply add a header to the request when creating the request instead of cloning etc., as we need to do for a proper roundtripper.

I did consider just adding the header when request is created, which is easier but more error-prone (need to remember to add the header again, if there's ever a new codepath that creates requests). That being said, I expect the exporter is fairly stable at this point, so this should be OK? f5bddaf

vreynolds · 2021-11-16T16:13:35Z

@bogdandrutu let me know if there's anything I need to do about the codecov report -- as far as I can tell, coverage is actually up, so I'm not sure what the failure is telling me

jpkrohling · 2021-11-17T10:20:20Z

I'm not sure what the failure is telling me

It's not a problem with this PR. It's just telling that the project as a whole is under the target of 90%, which has been the case since we brought the builder here.

codeboten · 2021-11-24T17:20:11Z

Closing and re-opening to kick the CI

codeboten · 2021-11-24T17:20:46Z

@open-telemetry/collector-maintainers please merge once CI passes, looks like we have all the approvals needed

jpkrohling · 2021-11-25T12:24:26Z

The new release created a conflict for this PR. Sorry about that, but could you rebase the changelog, @vreynolds?

vreynolds · 2021-11-30T18:21:11Z

@jpkrohling conflict resolved ✅

robbkidd · 2021-12-03T17:14:09Z

codeboten · 2021-12-03T17:36:02Z

@tigrannajaryan @bogdandrutu please merge 😬

feat: collector-aware user-agent header

3d72c7b

- add collector build info to otlp/otlphttp exporter headers

vreynolds requested review from a team and codeboten September 3, 2021 19:41

Aneurysm9 reviewed Sep 3, 2021

View reviewed changes

config/confighttp/confighttp.go Outdated Show resolved Hide resolved

config/configgrpc/configgrpc.go Outdated Show resolved Hide resolved

exporter/exporterhelper/common.go Outdated Show resolved Hide resolved

Address PR comments

fa5942c

- move user-agent additions to the exporters - add OS and ARCH to user-agent

Aneurysm9 approved these changes Sep 9, 2021

View reviewed changes

codeboten reviewed Sep 9, 2021

View reviewed changes

exporter/otlpexporter/otlp.go Outdated Show resolved Hide resolved

bogdandrutu reviewed Sep 13, 2021

View reviewed changes

config/confighttp/confighttp.go Outdated Show resolved Hide resolved

config/confighttp/confighttp.go Outdated Show resolved Hide resolved

exporter/otlphttpexporter/otlp.go Outdated Show resolved Hide resolved

github-actions bot added the Stale label Sep 21, 2021

bogdandrutu removed the Stale label Sep 21, 2021

codeboten approved these changes Sep 21, 2021

View reviewed changes

vreynolds added 3 commits September 21, 2021 12:43

Merge remote-tracking branch 'upstream/main' into collector-user-agent

13ad211

encase user-agent os/arch in parenthesis

8fd2bd7

add doc comment

c945d07

github-actions bot added the Stale label Sep 29, 2021

bogdandrutu removed the Stale label Sep 29, 2021

vreynolds added 5 commits October 6, 2021 11:46

move otlp-http user-agent to exporter

efd1fc1

formatting

672b7f6

revert unnecessary diff

0a86588

formatting

33bc09f

Merge remote-tracking branch 'upstream/main' into collector-user-agent

386a70b

vreynolds added 2 commits November 3, 2021 15:05

Merge remote-tracking branch 'upstream/main' into collector-user-agent

ab6adb1

http exporter: use a custom round tripper, clone request

0b44ddf

vreynolds changed the title ~~feat: collector-aware user-agent header~~ feat: informative user-agent for otlp exporters Nov 3, 2021

RoundTrip must always close the body

511b636

bogdandrutu approved these changes Nov 9, 2021

View reviewed changes

vreynolds added 2 commits November 10, 2021 12:49

remove custom roundtripper

f5bddaf

fix lint issue

b201e25

Merge remote-tracking branch 'upstream/main' into collector-user-agent

e232439

codeboten closed this Nov 24, 2021

codeboten reopened this Nov 24, 2021

update changelog

0ac32d6

Merge remote-tracking branch 'upstream/main' into collector-user-agent

a8bd4ee

jpkrohling approved these changes Dec 1, 2021

View reviewed changes

codeboten added the ready-to-merge Code review completed; ready to merge by maintainers label Dec 1, 2021

bogdandrutu approved these changes Dec 3, 2021

View reviewed changes

bogdandrutu merged commit acf6556 into open-telemetry:main Dec 3, 2021

alanwest mentioned this pull request Apr 29, 2022

OTLP exporters should add their version information as a resource attribute open-telemetry/opentelemetry-specification#2501

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: informative user-agent for otlp exporters #3970

feat: informative user-agent for otlp exporters #3970

vreynolds commented Sep 3, 2021

codeboten left a comment

vreynolds commented Sep 9, 2021

codeboten commented Sep 9, 2021

github-actions bot commented Sep 21, 2021

codeboten left a comment

vreynolds commented Sep 21, 2021

github-actions bot commented Sep 29, 2021

vreynolds commented Oct 4, 2021

codecov bot commented Oct 6, 2021 •

edited

Loading

vreynolds commented Nov 3, 2021

bogdandrutu left a comment

bogdandrutu Nov 9, 2021

vreynolds Nov 10, 2021

bogdandrutu Nov 9, 2021

vreynolds Nov 10, 2021

vreynolds commented Nov 16, 2021

jpkrohling commented Nov 17, 2021

codeboten commented Nov 24, 2021

codeboten commented Nov 24, 2021

jpkrohling commented Nov 25, 2021

vreynolds commented Nov 30, 2021

robbkidd commented Dec 3, 2021

codeboten commented Dec 3, 2021

feat: informative user-agent for otlp exporters #3970

feat: informative user-agent for otlp exporters #3970

Conversation

vreynolds commented Sep 3, 2021

codeboten left a comment

Choose a reason for hiding this comment

vreynolds commented Sep 9, 2021

codeboten commented Sep 9, 2021

github-actions bot commented Sep 21, 2021

codeboten left a comment

Choose a reason for hiding this comment

vreynolds commented Sep 21, 2021

github-actions bot commented Sep 29, 2021

vreynolds commented Oct 4, 2021

codecov bot commented Oct 6, 2021 • edited Loading

Codecov Report

vreynolds commented Nov 3, 2021

bogdandrutu left a comment

Choose a reason for hiding this comment

bogdandrutu Nov 9, 2021

Choose a reason for hiding this comment

vreynolds Nov 10, 2021

Choose a reason for hiding this comment

bogdandrutu Nov 9, 2021

Choose a reason for hiding this comment

vreynolds Nov 10, 2021

Choose a reason for hiding this comment

vreynolds commented Nov 16, 2021

jpkrohling commented Nov 17, 2021

codeboten commented Nov 24, 2021

codeboten commented Nov 24, 2021

jpkrohling commented Nov 25, 2021

vreynolds commented Nov 30, 2021

robbkidd commented Dec 3, 2021

codeboten commented Dec 3, 2021

codecov bot commented Oct 6, 2021 •

edited

Loading