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 AWS X-Ray Propagator #462

Merged
merged 8 commits into from
Dec 1, 2020
Merged

Add AWS X-Ray Propagator #462

merged 8 commits into from
Dec 1, 2020

Conversation

KKelvinLo
Copy link
Member

@KKelvinLo KKelvinLo commented Nov 21, 2020

Which problem is this PR solving?

This PR adds a propagator for propgating AWS X-Ray headers so that AWS X-Ray is able to receive and read traces from the OpenTelemetry Go SDK.

Short description of the changes

  • Updated Change Log with this change
  • Added AWS X-Ray Propagator
  • Added corresponding unit test for AWS X-Ray Propagator
  • Propagator conforms to OpenTelemetry propagator specifications with Inject and Extract defined as required

cc: @alolita

Copy link
Contributor

@jmacd jmacd left a comment

Choose a reason for hiding this comment

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

This looks like it copies parts of the ECS resource detector. Could you depend on it instead?

@KKelvinLo
Copy link
Member Author

@jmacd Hi Josh, I accidentally merged into our fork. I removed the ECS resource detector parts - could you take a look again? Thanks

Copy link
Member

@XSAM XSAM left a comment

Choose a reason for hiding this comment

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

Looks good overall.

propagators/awsxray/awsxray_propagator.go Outdated Show resolved Hide resolved
propagators/awsxray/awsxray_propagator.go Outdated Show resolved Hide resolved
propagators/awsxray/awsxray_propagator.go Outdated Show resolved Hide resolved
propagators/awsxray/awsxray_propagator.go Outdated Show resolved Hide resolved
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

The blocking issue here is the package name and the import path not matching.

propagators/awsxray/awsxray_propagator.go Outdated Show resolved Hide resolved
propagators/awsxray/awsxray_propagator.go Outdated Show resolved Hide resolved
propagators/awsxray/awsxray_propagator.go Outdated Show resolved Hide resolved
propagators/awsxray/awsxray_propagator.go Outdated Show resolved Hide resolved
propagators/awsxray/awsxray_propagator.go Outdated Show resolved Hide resolved
propagators/awsxray/awsxray_propagator.go Outdated Show resolved Hide resolved
propagators/awsxray/awsxray_propagator.go Outdated Show resolved Hide resolved
propagators/awsxray/awsxray_propagator.go Outdated Show resolved Hide resolved
propagators/awsxray/awsxray_propagator.go Outdated Show resolved Hide resolved
propagators/awsxray/awsxray_propagator.go Outdated Show resolved Hide resolved
* add AWS X-Ray propagator and tests

* made naming convention changes to adhere to linter

* added comment to interface implementation for  clarity

* update naming convention and added description for method
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

👍

propagators/aws/xray/propagator.go Outdated Show resolved Hide resolved
@MrAlias MrAlias merged commit e1c598c into open-telemetry:master Dec 1, 2020
plantfansam referenced this pull request in plantfansam/opentelemetry-go-contrib Mar 18, 2022
The methods on the `Float64Gauge`, `Int64Gauge`, `Float64Counter`,
`Int64Counter`, `Float64Measure`, and `Int64Measure` `struct`s do not
need to mutate the internal state of the `struct` and can therefore be
defined with value receivers instead. This aligns closer to the function
signatures of each instruments constructor function. Additionally, this
change means calls to these methods do not need an allocation to the
heap.

Resolves #440

Co-authored-by: Rahul Patel <rghetia@yahoo.com>
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.

None yet

6 participants