search

elixir-plug / plug_cowboy

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

[WIP] Telemetry via cowboy stream_handler #39

Closed binaryseed closed 4 years ago

binaryseed commented 4 years ago

WIP. This PR demonstrates how we might leverage a cowboy stream_handler to provide instrumentation that isn't susceptible to the problems that plug_adapter instrumentation encountered

This needs more work and an evaluation before consideration for merging

josevalim commented 4 years ago

This is absolutely beautiful. I have added just one comment, which is about either using [Next | StartTime] or a tuple. The cons is the smallest and fastest implementation of a pair in Erlang, but a tuple would be just fine. Other than that, I think we can totally move this to a separate package, as it probably has other uses outside Plug itself. Maybe a package inside beam-telemetry?

binaryseed commented 4 years ago

❤️ thanks! Updated to use cons, and also to exclude normal exits from the exception case...

I think a new package would be a good home for this - how should we go about getting that started?

josevalim commented 4 years ago

You have been invited to beam-telemetry so you can create the repo there. :)

binaryseed commented 4 years ago

I think responding based on Commands is better, but the new code is slightly less elegant :(

There's a nested case since I need to gather data from two of the commands in the list if they exist. The handlers in cowboy itself reduce over the list and accumulate some state, but I thought this was straightforward enough. Open to thoughts there

Commands that start with error_response have a few different shapes:

https://github.com/ninenines/cowboy/blob/db0d6f8d254f2cc01bd458dc41969e0b96991cc3/src/cowboy_stream_h.erl#L138

https://github.com/ninenines/cowboy/blob/db0d6f8d254f2cc01bd458dc41969e0b96991cc3/src/cowboy_stream_h.erl#L155

The upside of this is that the exception event now includes the actual response sent which none of the previous versions of this were able to do!

josevalim commented 4 years ago

Nice! We should probably get someone who actually understands Cowboy to review this too. :)

binaryseed commented 4 years ago

OK! I got this ported into it's own repo!

https://github.com/beam-telemetry/cowboy_telemetry

I just setup a basic Github Action for test, but nothing relating to publishing this as a package on Hex yet. I'm sure there's more to do getting it setup for release, but I'm closing this issue in favor of moving discussion to it's own repo & Issues

binaryseed commented 4 years ago

Once it gets published we can re-work this to reference the new package