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

feat: implement Span Links API #1269

Merged
merged 9 commits into from
Jul 22, 2022
Merged

Conversation

kruskall
Copy link
Member

This is an attempt to implement the Span Links API, mostly inspired by other PRs to the other agents and the spec/docs for OT.

Closes #1243

@apmmachine
Copy link
Contributor

apmmachine commented Jul 19, 2022

❕ Build Aborted

Either there was a build timeout or someone aborted the build.

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

Expand to view the summary

Build stats

  • Start Time: 2022-07-21T20:07:55.652+0000

  • Duration: 123 min 55 sec

Test stats 🧪

Test Results
Failed 0
Passed 6420
Skipped 148
Total 6568

Steps errors 3

Expand to view the steps failures

Build
  • Took 3 min 15 sec . View more details here
  • Description: ./scripts/jenkins/build.sh
Build
  • Took 3 min 37 sec . View more details here
  • Description: ./scripts/jenkins/build.sh
Build
  • Took 3 min 36 sec . View more details here
  • Description: ./scripts/jenkins/build.sh

🤖 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 19, 2022

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (59/59) 💚
Files 99.346% (152/153) 👍
Classes 96.264% (335/348) 👍 0.011
Methods 90.265% (955/1058) 👍 0.009
Lines 82.178% (11076/13478) 👎 -0.062
Conditionals 100.0% (0/0) 💚

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.

Looking good!

We'll need to add this for transactions too.

Can you please also add tests (one for spans, one for transactions)? We should show that the span links are included in the events sent, which we can verify using the apmtest.RecordingTracer. See span_test.go and transaction_test.go for examples. Since the spec says "Links SHOULD preserve the order in which they are set", it would be good to show that in the tests too.

span.go Outdated Show resolved Hide resolved
tracecontext.go Outdated Show resolved Hide resolved
@kruskall
Copy link
Member Author

kruskall commented Jul 20, 2022

Thank you for the feedback! 🙇

I've pushed some changes and added the tests.

We'll need to add this for transactions too.

Thank you for pointing this out, I reread the spec and I completely missed that part 🤦

@kruskall kruskall requested a review from axw July 20, 2022 14:06
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, looks good. I'd like to see the tests simplified, but otherwise LGTM.

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

Thanks for the feedback! I've pushed some changes 🏓

@kruskall kruskall requested a review from axw July 21, 2022 09:28
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! Just a suggestion about the test, I'll leave it to you to decide which way you prefer.

span_test.go Outdated Show resolved Hide resolved
@kruskall kruskall enabled auto-merge (squash) July 21, 2022 10:31
@kruskall
Copy link
Member Author

/test

@kruskall kruskall disabled auto-merge July 21, 2022 20:07
@kruskall
Copy link
Member Author

/test

@kruskall kruskall merged commit bc8805d into elastic:main Jul 22, 2022
@kruskall kruskall deleted the feat/span-links branch July 22, 2022 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[META 643] Implement Span Links API
3 participants