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

fix: filter traces #35

Conversation

novaugust
Copy link
Contributor

  • had the wrong key for stack_trace -> stacktrace
  • didn't call the filter traces function correctly :(

standby as i verify this branch

@novaugust novaugust force-pushed the error-handling-and-filter-traces-fixes branch from dee22e5 to b49ff7f Compare July 13, 2020 22:42
@novaugust
Copy link
Contributor Author

This definitely fixes the filter_traces bit. Can't know about the errors part until i get another error ^.^

@zachdaniel
Copy link
Member

just realized, in the error handling code and trace finishing code we should check the filter_traces to make sure that we don't close a trace we didn't start.

@novaugust
Copy link
Contributor Author

augh right, because finish_trace is called in two different spots >< thanks will add.

@zachdaniel
Copy link
Member

We should add that change in finish_trace as well.

@novaugust
Copy link
Contributor Author

finding the problem w/ error reporting. the conn isn't included in metadata at that point. @zachdaniel is using current_trace_id() a good way to find out if we're in a trace or not, and thus whether we should close it?

@zachdaniel
Copy link
Member

Did we ever find out of the stop and exception messages can both happen?

@zachdaniel
Copy link
Member

So current_trace_id() tells you that you are in a trace, but not necessarily if you are in the trace that you think you're in. Seems strange that there would be any point in the endpoint where the conn isn't available. Isn't that essentially the one constant data structure?

@novaugust
Copy link
Contributor Author

novaugust commented Jul 14, 2020

Seems strange that there would be any point in the endpoint where the conn isn't available. Isn't that essentially the one constant data structure?

You'd think right? And yet. No conn in metadata in phx exception. Docs specify the following:

%{kind: :throw | :error | :exit, reason: term(), stacktrace: Exception.stacktrace()}

but obviously other things are in there as well, or else my phx_controller? bit wouldn't work. Here's my stacktrace that tells me there's no conn:

Handler "spandex-router-telemetry" has failed and has been detached. 
Class=:error Reason={:badkey, :conn, %{error: ..., ....}

(stacktrace is included in error above, and cites the meta.conn call)

@novaugust
Copy link
Contributor Author

huh. source code in phx is:

https://github.com/phoenixframework/phoenix/blob/2fdf7a9d7e0d5504cb1115853ef26b06e45a4baf/lib/phoenix/router.ex#L359-L371

which shows that phx_action? shouldn't even work. now i'm confused how i'm getting to where i am

@novaugust
Copy link
Contributor Author

@zachdaniel re: phx telemetry router events. is it okay to call start_span/stop_span outside of a trace?

@novaugust
Copy link
Contributor Author

I'm still not seeing any error traces get reported, but have no errors in logs about telemetry disconnecting. Not sure what the story is.
Might be worth shipping this for the fix on filter_traces

@zachdaniel zachdaniel changed the title Error handling and filter traces fixes fix: filter traces Jul 15, 2020
@zachdaniel zachdaniel merged commit 7cedad1 into spandex-project:master Jul 15, 2020
@zachdaniel
Copy link
Member

Changes released as 1.0.3. We should really fix the error reporting though (or confirm that it works), so we can consider this nicely tied up :)

@zachdaniel
Copy link
Member

🚀 Thank you for your contribution! 🚀

@novaugust
Copy link
Contributor Author

@zachdaniel for sure, i really want it working xD I'll make an issue and keep evaluating. maybe we'll get lucky and someone else will see the problem (cough @keathley cough)

@novaugust novaugust deleted the error-handling-and-filter-traces-fixes branch July 15, 2020 15:23
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.

2 participants