elixir-plug / plug_cowboy

Plug adapter for the Cowboy web server
Other
243 stars 48 forks source link

Plug.Cowboy.Translator - add Plug.Conn to metadata? #55

Closed darrenclark closed 3 years ago

darrenclark commented 3 years ago

I'm currently working on a custom Logger backend to report crashes to a 3rd party service. For HTTP requests, I'd like to pull some metadata from the Plug.Conn and attach that to the report.

To get access to the conn in the Logger backend, I think it could be added to the metadata here:

https://github.com/elixir-plug/plug_cowboy/blob/a01f62928ca61a116dfee37578365ba97c1b07a5/lib/plug/cowboy/translator.ex#L40-L44

Thoughts?

If this change makes sense, I can open a PR.

josevalim commented 3 years ago

We could and a PR would be welcome (<3) but keep in mind that connection is the connection at the beginning of the request so it has limited amount of information.

darrenclark commented 3 years ago

Cool, will do!

keep in mind that connection is the connection at the beginning of the request so it has limited amount of information

Good to know, thanks.

I came across https://github.com/phoenixframework/phoenix/issues/2791#issuecomment-371548771 as well, I get the feeling I'll end up with duplicate crash reports though:

Is there a way I could de-duplicate those? Maybe comparing the Plug.Conn's owner pid? Is it always 1 pid per request?

josevalim commented 3 years ago

Recent Sentry skips this particular logging based on the current metadata (it has an option that does so), so you should not see double logging (unless your custom logger backend also sends it to sentry :P)

darrenclark commented 3 years ago

Not using Sentry at the moment, but maybe I should be! :D

josevalim commented 3 years ago

You can just replicate their behavior too. They have a plug that wraps the request life-cycle, so they don't rely on this particular Cowboy report and instead they just ignore it. Handling this from the request life-cycle has benefits as you generally have a more up-to-date connection.