Skip to content

implementation for Trace Continuation Strategy#2760

Merged
jackshirazi merged 12 commits intoelastic:mainfrom
jackshirazi:add-trace_continuation_strategy
Sep 7, 2022
Merged

implementation for Trace Continuation Strategy#2760
jackshirazi merged 12 commits intoelastic:mainfrom
jackshirazi:add-trace_continuation_strategy

Conversation

@jackshirazi
Copy link
Copy Markdown
Contributor

What does this PR do?

Implements Trace Continuation Strategy per https://github.com/elastic/apm/blob/main/specs/agents/trace-continuation.md

Note for reviewers. The choice of Transaction.onTransactionStart() as the sole location to apply the strategy is because

  1. The headers are processed in TraceContext, so would be the ideal place to apply the strategy, but it doesn't have access to
    the span for adding span links, so the options were to
    1a. Reimplement all the trace context header handling to enable that or
    1b. Move up the stack - which is what I chose
  2. Moving up the stack, I analyzed all the callers of TraceConext.getFromTraceContextTextHeaders() and getFromTraceContextBinaryHeaders(). These were either in tests (ignorable), or from plugins adding spanlinks (ignorable). Of the remaining two
    2a ExternalSpanContextInstrumentation.parseTextMap is for reading headers only, so can be ignored
    2b JmsInstrumentationHelper.makeChildOf which uses JMS headers so I've assumed that the strategy doesn't apply here (THIS MAY BE AN INCORRECT ASSUMPTION)
  3. Consequently places in Transaction which call those two TraceContext methods both funnel through Transaction.onTransactionStart() so this handles both binary and text headers

Checklist

  • This is an enhancement of existing features, or a new feature in existing plugins
    • I have updated CHANGELOG.asciidoc
    • I have added tests that prove my fix is effective or that my feature works
    • Added an API method or config option? Document in which version this will be introduced
    • I have made corresponding changes to the documentation
  • This is a bugfix
  • This is a new plugin
    • I have updated CHANGELOG.asciidoc
    • My code follows the style guidelines of this project
    • I have made corresponding changes to the documentation
    • I have added tests that prove my fix is effective or that my feature works
    • New and existing unit tests pass locally with my changes
    • I have updated supported-technologies.asciidoc
    • Added an API method or config option? Document in which version this will be introduced
    • Added an instrumentation plugin? Describe how you made sure that old, non-supported versions are not instrumented by accident.
  • This is something else

@jackshirazi jackshirazi requested a review from a team August 26, 2022 15:12
@ghost
Copy link
Copy Markdown

ghost commented Aug 26, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-09-07T09:47:31.217+0000

  • Duration: 62 min 32 sec

Test stats 🧪

Test Results
Failed 0
Passed 3138
Skipped 36
Total 3174

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark tests.

  • run jdk compatibility tests : Run the JDK Compatibility tests.

  • run integration tests : Run the Agent Integration tests.

  • run end-to-end tests : Run the APM-ITs.

  • run windows tests : Build & tests on windows.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

Copy link
Copy Markdown
Member

@felixbarny felixbarny 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 👏
Just a couple of small suggestions

Comment thread CHANGELOG.asciidoc Outdated
jackshirazi and others added 8 commits August 30, 2022 12:23
…tion/Transaction.java

Co-authored-by: Felix Barnsteiner <felixbarny@users.noreply.github.com>
…n/CoreConfiguration.java

Co-authored-by: Felix Barnsteiner <felixbarny@users.noreply.github.com>
Co-authored-by: Felix Barnsteiner <felixbarny@users.noreply.github.com>
@jackshirazi jackshirazi enabled auto-merge (squash) September 7, 2022 10:02
@jackshirazi jackshirazi merged commit 565c2e8 into elastic:main Sep 7, 2022
@AlexanderWert AlexanderWert linked an issue Sep 12, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[META 604] Implement trace_continuation_strategy

3 participants