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 ID Generator #459

Merged
merged 14 commits into from
Dec 14, 2020
Merged

Conversation

wilguo
Copy link
Contributor

@wilguo wilguo commented Nov 18, 2020

This PR adds an ID Generator for sending traces to AWS X-Ray as well as unit tests for testing the ID Generator.

The core functionalities include:

  • NewTraceID() which generates a new Trace ID based on AWS X-Ray TraceID Format
  • NewSpanID() which generates a new Span ID from a randomly-chosen sequence

cc @alolita

Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

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

Please create a separate module for idgenerator/aws. This should ensure that it can be imported independently and doesn't result in additional imports for users of contrib packages that don't use it.

idgenerator/aws/aws_xray_idgenerator.go Outdated Show resolved Hide resolved
idgenerator/aws/aws_xray_idgenerator.go Outdated Show resolved Hide resolved
@wilguo
Copy link
Contributor Author

wilguo commented Nov 18, 2020

Hi @Aneurysm9,

I've added a separate module for idgenerator/aws however now it's giving me an error when I run make precommit.

missing go.mod dependabot check:
./idgenerator/aws
make: *** [dependabot-check] Error 1

I'm assuming I didn't add a line for the dependabot somewhere, but not sure where to add this check. Could you point me in the right direction? Thank you!

@Aneurysm9
Copy link
Member

I'm assuming I didn't add a line for the dependabot somewhere, but not sure where to add this check. Could you point me in the right direction? Thank you!

You'll need to update .github/dependabot.yml to include an entry for the new module.

@wilguo wilguo requested a review from Aneurysm9 November 18, 2020 23:38
idgenerator/aws/aws_xray_idgenerator.go Outdated Show resolved Hide resolved
idgenerator/aws/aws_xray_idgenerator.go Outdated Show resolved Hide resolved
idgenerator/aws/aws_xray_idgenerator.go Outdated Show resolved Hide resolved
@wilguo wilguo requested a review from Aneurysm9 November 19, 2020 20:59
CHANGELOG.md Outdated Show resolved Hide resolved
idgenerator/aws/aws_xray_idgenerator.go Outdated Show resolved Hide resolved
idgenerator/aws/aws_xray_idgenerator.go Outdated Show resolved Hide resolved
idgenerator/aws/xray/aws_xray_idgenerator.go Outdated Show resolved Hide resolved
idgenerator/aws/xray/aws_xray_idgenerator.go Outdated Show resolved Hide resolved
idgenerator/aws/xray/aws_xray_idgenerator.go Outdated Show resolved Hide resolved
idgenerator/aws/xray/aws_xray_idgenerator.go Outdated Show resolved Hide resolved
idgenerator/aws/xray/aws_xray_idgenerator.go Outdated Show resolved Hide resolved
idgenerator/aws/xray/aws_xray_idgenerator.go Outdated Show resolved Hide resolved
wilguo added a commit to open-o11y/opentelemetry-go-contrib that referenced this pull request Nov 23, 2020
Copy link

@rakyll rakyll left a comment

Choose a reason for hiding this comment

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

Thanks, this generally looks good. I left a comment about the package's place.

idgenerator/aws/xray/aws_xray_idgenerator.go Outdated Show resolved Hide resolved
idgenerator/aws/xray/aws_xray_idgenerator.go Outdated Show resolved Hide resolved
.github/dependabot.yml Outdated Show resolved Hide resolved
wilguo referenced this pull request in open-o11y/opentelemetry-go-contrib Nov 25, 2020
idgenerator/aws/xray/aws_xray_idgenerator.go Outdated Show resolved Hide resolved
wilguo added a commit to open-o11y/opentelemetry-go-contrib that referenced this pull request Nov 27, 2020
wilguo referenced this pull request in open-o11y/opentelemetry-go-contrib Nov 27, 2020
@wilguo wilguo force-pushed the wilguo-idgenerator branch from 1b5a5c9 to 62a60ba Compare November 27, 2020 23:30
@wilguo wilguo requested a review from MrAlias November 27, 2020 23:44
@wilguo wilguo mentioned this pull request Nov 28, 2020
@alolita
Copy link
Member

alolita commented Dec 2, 2020

@MrAlias @Aneurysm9 can you please re-review @wilguo's changes and merge? We're blocked on this. Ty!

MrAlias
MrAlias previously requested changes Dec 3, 2020
go.sum Outdated Show resolved Hide resolved
propagators/aws/xray/idgenerator.go Show resolved Hide resolved
propagators/aws/xray/idgenerator.go Outdated Show resolved Hide resolved
propagators/aws/xray/idgenerator_test.go Outdated Show resolved Hide resolved
propagators/aws/xray/idgenerator_test.go Outdated Show resolved Hide resolved
propagators/aws/xray/idgenerator_test.go Outdated Show resolved Hide resolved
propagators/aws/xray/idgenerator_test.go Outdated Show resolved Hide resolved
propagators/aws/xray/idgenerator_test.go Outdated Show resolved Hide resolved
propagators/aws/xray/idgenerator_test.go Outdated Show resolved Hide resolved
propagators/aws/xray/idgenerator_test.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@wilguo
Copy link
Contributor Author

wilguo commented Dec 11, 2020

@Aneurysm9 would it be the same if I make a new commit resolving all the conflicts?

possibly, though probably safer to revert and re-apply.

@Aneurysm9 I have reverted the commit, but the circlci is failing, is there a step I'm missing?

wilguo added a commit to open-o11y/opentelemetry-go-contrib that referenced this pull request Dec 11, 2020
wilguo referenced this pull request in open-o11y/opentelemetry-go-contrib Dec 11, 2020
@wilguo wilguo force-pushed the wilguo-idgenerator branch 2 times, most recently from 40454ab to fb1ebbf Compare December 11, 2020 22:36
@wilguo wilguo force-pushed the wilguo-idgenerator branch from fb1ebbf to 0156c0b Compare December 11, 2020 23:16
@wilguo wilguo force-pushed the wilguo-idgenerator branch from 0156c0b to 88631cd Compare December 11, 2020 23:25
@MrAlias MrAlias dismissed their stale review December 14, 2020 16:02

not relevant anymore

propagators/aws/go.mod Outdated Show resolved Hide resolved
propagators/aws/xray/idgenerator_test.go Outdated Show resolved Hide resolved
propagators/aws/xray/idgenerator_test.go Outdated Show resolved Hide resolved
propagators/aws/xray/idgenerator_test.go Outdated Show resolved Hide resolved
propagators/aws/xray/idgenerator_test.go Outdated Show resolved Hide resolved
propagators/aws/xray/idgenerator_test.go Outdated Show resolved Hide resolved
propagators/aws/xray/idgenerator_test.go Outdated Show resolved Hide resolved
propagators/aws/xray/idgenerator_test.go Outdated Show resolved Hide resolved
wilguo and others added 8 commits December 14, 2020 08:25
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@wilguo wilguo requested a review from MrAlias December 14, 2020 17:16
@MrAlias MrAlias merged commit f899260 into open-telemetry:master Dec 14, 2020
plantfansam referenced this pull request in plantfansam/opentelemetry-go-contrib Mar 18, 2022
* add simplified export pipeline setup for Jaeger

* add With* options to configure SDK options.

* add test for WithRegistration and WithSDK

* rename Registeration with RegisterGlobal

* rename WithRegistration to RegisterAsGlobal

Co-authored-by: rahulpa <rahulpa@google.com>
Co-authored-by: Joshua MacDonald <jmacd@users.noreply.github.com>
@pellared pellared added this to the untracked milestone Nov 8, 2024
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.

10 participants