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

Update Doc for aws propagator default sampling behavior #656

Merged

Conversation

NathanielRN
Copy link
Contributor

@NathanielRN NathanielRN commented Sep 2, 2021

Description

By default the TraceProvider uses a Sampler which respects the Parent Sampling decision. If the user does not provide Sampled=1; into their X-Amzn-Trace-Id header when using the AwsXrayFormat propagator, the parent context (and all subsequent child spans) are assumed to be sampled=false by default.

This PR updates the docs to warn users about that.

Fixes #649

Type of change

Please delete options that are not relevant.

  • This change requires a documentation update

How Has This Been Tested?

N/A

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
    - [ ] Changelogs have been updated
    - [ ] Unit tests have been added
  • Documentation has been updated

@NathanielRN NathanielRN requested a review from a team as a code owner September 2, 2021 20:21
@NathanielRN NathanielRN added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Sep 2, 2021
@owais
Copy link
Contributor

owais commented Sep 2, 2021

Can you review your own PR? :) @NathanielRN ?

@NathanielRN
Copy link
Contributor Author

Ah that's a good point @owais 🙃 That's probably why the CODEOWNERS reviewer didn't work 😅 I'll close this then 🙂

@NathanielRN NathanielRN closed this Sep 2, 2021
@NathanielRN NathanielRN changed the title Update DOC for aws propagator default sampling behavior Update Doc for aws propagator default sampling behavior Sep 2, 2021
@owais
Copy link
Contributor

owais commented Sep 2, 2021

I can still merge it for you as a maintainer if you re-open

@owais
Copy link
Contributor

owais commented Sep 2, 2021

Until update the check to only require it if you are not the author or add another AWS person as codeowner on your recommendation.

@NathanielRN
Copy link
Contributor Author

Thanks a ton for that! 😄 I think I can get a colleague at AWS to open the PR for me so we can test the CODEOWNER action out, but I appreciate your help with this! 🙂

@NathanielRN
Copy link
Contributor Author

@lzchen An update on the CODEOWNERS action. With the change in #659 I will now get assigned even if I am the one who opened the PR (I still can't review though). However, I noticed that if I try to amend a commit, the "Component Owner" action fails as seen on this workflow.

I filed this issue on the action repo and mentioned that maybe the solution is to not run on the pull_request_target -> synchronize activity (which is a default activity of the GitHub event).

@NathanielRN NathanielRN force-pushed the document-parent-context-behavior branch from f6c8ba6 to d96681a Compare September 8, 2021 17:51
@lzchen lzchen merged commit 97e9f2f into open-telemetry:main Sep 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Providing Parent in X-Amzn-Trace-Id results in no spans being exported
3 participants