Skip to content
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

Add collector version to xray exporter user agent #1730

Merged

Conversation

anuraaga
Copy link
Contributor

@anuraaga anuraaga commented Dec 1, 2020

Description: To provide support to customers using the otel collector, we need some more telemetry data from the collector. Adding the collector version to user agent is the first simple step.

As follow up, I would

  • Extract this into a shared package and use from all the AWS clients (probably a good time to extract all the STS copy-paste too)

  • Add version.DistributionType = "opentelemetry-collector-contrib" and add that too. Custom distributions can replace that with e.g., aws-otel-collector

Testing: Unit tests

@anuraaga anuraaga requested a review from kbrockhoff as a code owner December 1, 2020 07:39
@anuraaga anuraaga requested a review from a team December 1, 2020 07:39
@anuraaga
Copy link
Contributor Author

anuraaga commented Dec 1, 2020

/cc @mxiamxia

@@ -53,6 +56,11 @@ const (
STSAwsCnPartitionIDSuffix = ".amazonaws.com.cn" // AWS China partition.
)

var collectorUserAgentHandler = request.NamedHandler{
Name: "otel.collector.UserAgentHandler",
Fn: request.MakeAddToUserAgentHandler("opentelemetry-collector-contrib", version.Version, version.GitHash),
Copy link
Member

@mxiamxia mxiamxia Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Trying to have better understanding. where and who can set version.DistributionType = "opentelemetry-collector-contrib" to replace the value? Just another thought, can we consider to use env variable to replace the default collector name "opentelemetry-collector-contrib" and collector version? In that case, AWS OTel Collector can set customized value for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and added the distributionType variable in this PR so that would be modifiable with ldflags

Anuraag Agrawal added 2 commits December 2, 2020 10:41
@codecov
Copy link

codecov bot commented Dec 2, 2020

Codecov Report

Merging #1730 (0470a8c) into master (24951cc) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1730      +/-   ##
==========================================
+ Coverage   89.57%   89.58%   +0.01%     
==========================================
  Files         375      375              
  Lines       18294    18299       +5     
==========================================
+ Hits        16387    16394       +7     
+ Misses       1413     1411       -2     
  Partials      494      494              
Flag Coverage Δ
integration 70.97% <ø> (ø)
unit 88.29% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
exporter/awsxrayexporter/awsxray.go 67.92% <100.00%> (+0.61%) ⬆️
exporter/awsxrayexporter/factory.go 100.00% <100.00%> (ø)
exporter/awsxrayexporter/xray_client.go 76.00% <100.00%> (+4.57%) ⬆️
processor/groupbytraceprocessor/event.go 95.96% <0.00%> (-0.81%) ⬇️
receiver/prometheusexecreceiver/receiver.go 88.33% <0.00%> (+2.50%) ⬆️

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 24951cc...0470a8c. Read the comment docs.

exporter/awsxrayexporter/awsxray.go Outdated Show resolved Hide resolved
exporter/awsxrayexporter/awsxray.go Outdated Show resolved Hide resolved
Copy link
Member

@mxiamxia mxiamxia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants