[WIP] Telemetry via cowboy stream_handler

[binaryseed]

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

[binaryseed]

stream_handler is still a bit of an implementation detail, and it's not true that all stream handler events are emitted, so I'm thinking that [cowboy, request, start|stop|exception] would be ideal...

[josevalim]

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]

❤️ 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]

Reason /= normal means no exception nor stop events will be emitted. I am not sure that's the way we want to go. Another other ideas?

[binaryseed]

I'm actually noticing that other handlers (like cowboy_metrics_h) respond to the Commands rather than the Info.. And I've found that an error_response tuple is part of the commands returned alongside an exit - it's shaped just like the response tuple.

Commands:

• request process completes:
  • [{response, _, _, _}]
• request process fails:
  • [{error_response, _, _, _}, _log_command, {internal_error, {EXIT, pid, reason}}]

Now I'm thinking of reworking this so that the exception event is all based on Commands. It seems better to respond based on the commands cowboy gives rather than trying to pre-parse the info.

I'll push up a change for that soon...

[josevalim]

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

[binaryseed]

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]

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

[binaryseed]

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]

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