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

[BUG] The thread context is not properly cleared and messes up the traces #10873

Merged
merged 3 commits into from
Nov 20, 2023

Conversation

reta
Copy link
Collaborator

@reta reta commented Oct 23, 2023

Description

The thread context stashing messes up the propagation of the current span across thread boundaries (and even same thread boundary). It leads to split brain situation when some state is stored in ThreadContext and in the thread local scope, which is difficult (if possible at all) to reconcile.

Related Issues

Closes #10789

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)
  • Public documentation issue/PR created

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@github-actions github-actions bot added bug Something isn't working v2.12.0 Issues and PRs related to version 2.12.0 v3.0.0 Issues and PRs related to version 3.0.0 labels Oct 23, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 23, 2023

Compatibility status:

Checks if related components are compatible with change 7b1e67e

Incompatible components

Incompatible components: [https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/performance-analyzer.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/performance-analyzer-rca.git]

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@reta reta force-pushed the issue-10789 branch 2 times, most recently from dbc073e to 8a06238 Compare October 24, 2023 14:08
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@reta
Copy link
Collaborator Author

reta commented Oct 25, 2023

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (54ff353) 71.09% compared to head (7b1e67e) 71.14%.
Report is 13 commits behind head on main.

Files Patch % Lines
.../java/org/opensearch/common/settings/Settings.java 77.77% 1 Missing and 3 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #10873      +/-   ##
============================================
+ Coverage     71.09%   71.14%   +0.05%     
- Complexity    58752    58785      +33     
============================================
  Files          4888     4888              
  Lines        277207   277221      +14     
  Branches      40282    40288       +6     
============================================
+ Hits         197077   197226     +149     
+ Misses        63654    63503     -151     
- Partials      16476    16492      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@reta reta marked this pull request as ready for review October 26, 2023 02:00
@reta
Copy link
Collaborator Author

reta commented Oct 26, 2023

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.search.SearchWeightedRoutingIT.testSearchAggregationWithNetworkDisruption_FailOpenEnabled

@Gaganjuneja
Copy link
Contributor

Looking into it.

…aces

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
Copy link
Contributor

✅ Gradle check result for 5fe42cd: SUCCESS

@reta
Copy link
Collaborator Author

reta commented Nov 17, 2023

@andrross may I ask you please to approve, we worked with @Gaganjuneja to confirm the "mess" it gone for now but the work will continue, thank you.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

There should be a unit test for DefaultSpanScope that ensures attach/detach works in the various scenarios (e.g. exception).

CHANGELOG.md Outdated Show resolved Hide resolved
@reta
Copy link
Collaborator Author

reta commented Nov 20, 2023

There should be a unit test for DefaultSpanScope that ensures attach/detach works in the various scenarios (e.g. exception).

We do have tests for that actually, the reason why everything still work is that the DefaultSpanScope was always manipulated this way (when created): DefaultSpanScope.create(...).attach() so functionally there are no changes in these regards.

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
Copy link
Contributor

✅ Gradle check result for 7b1e67e: SUCCESS

@reta reta merged commit 00517eb into opensearch-project:main Nov 20, 2023
29 checks passed
@reta reta added the backport 2.x Backport to 2.x branch label Nov 20, 2023
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-2.x
# Create a new branch
git switch --create backport/backport-10873-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 00517eb21144065bd779c1777e723e5d8c1f0ecb
# Push it to GitHub
git push --set-upstream origin backport/backport-10873-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-10873-to-2.x.

reta added a commit to reta/OpenSearch that referenced this pull request Nov 20, 2023
…aces (opensearch-project#10873)

* [BUG] The thread context is not properly cleared and messes up the traces

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

* Address code review comments

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

* Address code review comments

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

---------

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
(cherry picked from commit 00517eb)
andrross pushed a commit that referenced this pull request Nov 21, 2023
…aces (#10873) (#11277)

* [BUG] The thread context is not properly cleared and messes up the traces

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

* Address code review comments

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

* Address code review comments

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

---------

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
(cherry picked from commit 00517eb)

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
fahadshamiinsta pushed a commit to fahadshamiinsta/OpenSearch270 that referenced this pull request Dec 4, 2023
…aces (opensearch-project#10873)

* [BUG] The thread context is not properly cleared and messes up the traces

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

* Address code review comments

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

* Address code review comments

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

---------

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
rayshrey pushed a commit to rayshrey/OpenSearch that referenced this pull request Mar 18, 2024
…aces (opensearch-project#10873)

* [BUG] The thread context is not properly cleared and messes up the traces

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

* Address code review comments

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

* Address code review comments

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

---------

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
…aces (opensearch-project#10873)

* [BUG] The thread context is not properly cleared and messes up the traces

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

* Address code review comments

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

* Address code review comments

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

---------

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
Signed-off-by: Shivansh Arora <hishiv@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch backport-failed bug Something isn't working v2.12.0 Issues and PRs related to version 2.12.0 v3.0.0 Issues and PRs related to version 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] The thread context is not properly cleared and messes up the traces
3 participants