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.

Sign up for GitHub

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

Jump to bottom

feat: implement trace_continuation_strategy #1270

Merged
merged 6 commits into from
Jul 26, 2022
Merged

feat: implement trace_continuation_strategy #1270

merged 6 commits into from
Jul 26, 2022

Conversation

kruskall
Copy link
Member

@kruskall kruskall commented Jul 22, 2022 •
edited
Loading

Add ELASTIC_APM_TRACE_CONTINUATION_STRATEGY variable to change the
continuation strategy. (Default to 'continue')

Add test to verify trace ids are different and span links are
populated properly.

Closes #1221

@kruskall
feat: implement trace_continuation_strategy
a58266d 
Add ELASTIC_APM_TRACE_CONTINUATION_STRATEGY variable to change the
continuation strategy. (Default to 'continue')

Add test to verify trace ids are different and span links are
populated properly.
@apmmachine
Copy link
Contributor

apmmachine commented Jul 22, 2022 •
edited
Loading

💚 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-07-26T10:04:57.498+0000

  • Duration: 49 min 0 sec

Test stats 🧪

Test Results
Failed 0
Passed 7459
Skipped 174
Total 7633

🤖 GitHub comments

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

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark test.

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

@apmmachine
Copy link
Contributor

apmmachine commented Jul 22, 2022 •
edited
Loading

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (59/59) 💚
Files 99.346% (152/153) 👍
Classes 96.264% (335/348) 👍
Methods 90.274% (956/1059) 👍 0.028
Lines 82.118% (11090/13505) 👎 -0.008
Conditionals 100.0% (0/0) 💚

axw
axw requested changes Jul 25, 2022
View reviewed changes
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

Looks great, just a couple of comments on the tests.

Can you please also add a subtest to TestTracerCentralConfigUpdate in config_test.go?

config.go Outdated Show resolved Hide resolved
module/apmhttp/handler_test.go Outdated Show resolved Hide resolved
kruskall added 2 commits July 25, 2022 19:05
@kruskall
refactor: avoid returning unused value
fb007d9 
parseContinuationStrategy was not actually parsing the value, update
the method to reflect that.
@kruskall
test: move continuation strategy test to apm package
6872aac 
migrate test to a subtests with coverage for all the different
strategies.
The special case (restart_external + missing header) is also
tested.
@kruskall kruskall requested a review from axw July 25, 2022 21:03
axw
axw requested changes Jul 26, 2022
View reviewed changes
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

Thanks, the new test looks great. ICYMI:

Can you please also add a subtest to TestTracerCentralConfigUpdate in config_test.go?

Then this will be good to merge.

transaction_test.go Show resolved Hide resolved
@kruskall
Copy link
Member Author

Thank you for the feedback! 🙇

I added the config test and pushed a few changes 🏓

@kruskall kruskall requested a review from axw July 26, 2022 11:23
axw
axw approved these changes Jul 26, 2022
View reviewed changes
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

LGTM!

@kruskall kruskall merged commit e9b1bd5 into elastic:main Jul 26, 2022
@kruskall kruskall deleted the feat/continuation-strategy branch July 26, 2022 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@axw axw axw approved these changes

Assignees
No one assigned
Labels
agent-go
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[META 604] Implement trace_continuation_strategy
3 participants
@kruskall @apmmachine @axw