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

Mark some more spans as exit spans #1334

Merged
merged 5 commits into from
Oct 17, 2022
Merged

Mark some more spans as exit spans #1334

merged 5 commits into from
Oct 17, 2022

Conversation

axw
Copy link
Member

@axw axw commented Oct 13, 2022

Also, note a bug in Span.End() when ending a child of an exit span.

Relates to #1315

Also, note a bug in Span.End() when ending a child of an exit span.
@apmmachine
Copy link
Contributor

apmmachine commented Oct 13, 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-10-17T06:41:22.911+0000

  • Duration: 53 min 9 sec

Test stats 🧪

Test Results
Failed 0
Passed 8554
Skipped 201
Total 8755

🤖 GitHub comments

Expand to view the 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!)

@axw
Copy link
Member Author

axw commented Oct 13, 2022

/test

span.go Outdated
@@ -370,6 +370,9 @@ func (s *Span) End() {
s.Context.model.Destination = nil
s.Context.model.Service = nil

// BUG(axw) s.parent.SpanData must be accessed with s.parent.mu held.
// Even then, s.parent.SpanData will be nil if ended, so we need to
// find an alternative approach.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind creating a gh issue for that bug? As I understand this won't be fixed within the same PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was intending to fix it in this PR. The other changes could slip to another release, but not this; this causes a panic if you end the parent before the child.

@axw axw requested a review from simitt October 17, 2022 02:52
@axw axw marked this pull request as ready for review October 17, 2022 02:52
@axw
Copy link
Member Author

axw commented Oct 17, 2022

I haven't updated CHANGELOG, as the bug hasn't made it into a release.

@apmmachine
Copy link
Contributor

apmmachine commented Oct 17, 2022

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (59/59) 💚
Files 99.346% (152/153)
Classes 96.275% (336/349)
Methods 90.49% (961/1062)
Lines 82.291% (11227/13643)
Conditionals 100.0% (0/0) 💚

@axw
Copy link
Member Author

axw commented Oct 17, 2022

/test

@axw axw enabled auto-merge (squash) October 17, 2022 06:53
@axw axw merged commit 09c1fe1 into elastic:main Oct 17, 2022
@axw axw deleted the exitspan-fixes branch October 17, 2022 08:45
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.

3 participants