-
Notifications
You must be signed in to change notification settings - Fork 4.3k
chore(applicationsignals-alpha): remove B3 from default propagators #35510
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This review is outdated)
4328317
to
364c267
Compare
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
714055b
to
3253cb7
Compare
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
1 similar comment
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
This pull request has been removed from the queue for the following reason: The pull request can't be updated. You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
B3 propagator currently drops trace state and baggage if it comes after either of the propagators responsible for propagating these informations. It provides no functionality and only blocks baggage in the current state.
8e422af
to
67a4e24
Compare
Pull request has been modified.
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
1 similar comment
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Pull request has been modified.
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
1 similar comment
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
This pull request has been removed from the queue for the following reason: Pull request #35510 has been dequeued. Mergify failed to merge the pull request. GitHub can't merge the pull request after 15 retries. You can check the last failing draft PR here: #35594. You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Pull request has been modified.
Not sure what's going on here... I have "Allow edits by maintainers" set appropriately and the branch is not protected or anything. I tried rebasing manually the first time and then used the "Update branch" button this time and it resulted in the same, dequeueing my PR and dismissing the approval |
@Mergifyio refresh |
✅ Pull request refreshed |
Our merge tooling is acting up. Our team is already taking a look |
@Mergifyio refresh |
✅ Pull request refreshed |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
1 similar comment
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
N/A
Reason for this change
B3 propagator currently drops trace state and baggage if it comes after either of the propagators responsible for propagating these informations. It provides no functionality and only blocks baggage in the current state.
This blocks certain AWS X-Ray features from being used effectively without asking users to override the default propagators
Description of changes
Describe any new or updated permissions being added
N/A
Description of how you validated changes
Confirmed with SME wangzlei@ that B3 is no longer needed, as per owners listed in https://github.com/open-telemetry/opentelemetry-java-contrib/tree/main/aws-xray-propagator
Ran manual tests by deploying applications hosted in both EC2 and EKS with various propagator configurations that exclude B3 using Application Signals on Java and verified all Application Signals behaviour was functional and as expected.
Removed the B3 default propagator in ADOT Java that is applied in non-CDK environments and verified the Application Signals E2E tests passed: https://github.com/aws-observability/aws-otel-java-instrumentation/actions/runs/17587061009/job/49960543385
(Failed jobs are due to misconfiguration of unrelated actions)
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license