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

Close span instead of trace on router exception #37

Merged
merged 1 commit into from
Sep 3, 2020

Conversation

bforchhammer
Copy link
Contributor

@sourcelevel-bot
Copy link

SourceLevel has finished reviewing this Pull Request and has found:

  • 1 fixed issue! 🎉

See more details about this review.

@zachdaniel
Copy link
Member

This looks good to me. @GregMefford thoughts?

Copy link
Contributor

@novaugust novaugust left a comment

Choose a reason for hiding this comment

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

Spandex's internals aren't clear to me, so I'm not sure why finishing the span instead of the trace fixes things, but I'm happy if it does! This bug's been bugging me

@bforchhammer
Copy link
Contributor Author

Well, basically spandex hooks itself up to 2 types of telemtry events: "endpoint" events and "router" events, and the "endpoint start" event is used to start a trace, the "endpoint stop" event is used to attach common metadata as well as close the trace.

The router events (start/stop/exception) are always emitted in between the endpoint start and endpoint stop. The "router start" event starts a new span, and stop/exception should close that span.

The issue which this PR fixes is that currently the "exception" event closes the full trace instead of just closing the span, which means the "endpoint stop" event doesn't have a closable trace anymore and cannot attach metadata...

@novaugust
Copy link
Contributor

Right, but does this then fix the problem where no error traces are showing up in datadog?

(ps: it use to add metadata in exceptions as well, but phx had a bug where they didn't include the conn in an exception so that didn't work! they fixed that in 1.5.4 though)

@bforchhammer
Copy link
Contributor Author

bforchhammer commented Sep 1, 2020

Right, but does this then fix the problem where no error traces are showing up in datadog?

This PR solves exception traces not having the right tags, which includes the resource name (ie. for me exception traces were not grouped together with successful traces for the same resource); I also stumbled on another issue with traces not being "marked as errors", see #24 (comment)

@novaugust
Copy link
Contributor

(ie. for me exception traces were not grouped together with successful traces for the same resource)

kaching. that makes a ton of sense as the reason why nothing was showing up. i thought no errors were being sent at all, but prompted by your comment i dug into Traces search on datadog and found them. many thanks! excited for this to get in

Copy link
Member

@GregMefford GregMefford left a comment

Choose a reason for hiding this comment

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

Just checked this against the internal equivalent that I've been using for this and confirmed this is correct! I also see that we have a couple more improvements internally that I can work on contributing here as a separate PR. 🚀

@GregMefford GregMefford merged commit 00bd083 into spandex-project:master Sep 3, 2020
@GregMefford
Copy link
Member

Oh and thank you all for finding this issue and taking the time to contribute a fix / peer-review each others' work, and everything! Y'all are great! ❤️

@bforchhammer bforchhammer deleted the phoenix15fixes branch September 3, 2020 15:14
@novaugust
Copy link
Contributor

could we get a release with this? ❤️ ❤️

@zachdaniel
Copy link
Member

I'll build one.

@zachdaniel
Copy link
Member

Released as 1.0.4
🚀 Thank you for your contribution! 🚀

@novaugust
Copy link
Contributor

zach you're a gosh damned hero, thanks man

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants