search

beam-telemetry / cowboy_telemetry

Telemetry instrumentation for Cowboy
Apache License 2.0
30 stars 9 forks source link

Missing commands #5

Closed essen closed 4 years ago

essen commented 4 years ago

I've done a quick review of the handler. I see that you want to get start/end times for producing responses, but you're not handling all cases where a response ends here:

https://github.com/beam-telemetry/cowboy_telemetry/blob/main/src/cowboy_telemetry_h.erl#L28

In the case of Websocket the response will end on a 101 inform command.

When the headers command is used, the body is streamed, and the body is fully sent once the data command is used with fin as the second element of the tuple OR when the trailers frame is sent.

The internal_error will also result in a response being sent, if one was not sent already, but only for HTTP/1.1. In that case a 500 is sent. In HTTP/2's case a stream error will be sent.

If you can decouple tracing and metrics handling I would strongly suggest using cowboy_metrics_h for the metrics as you'll get a lot of useful information from there.

binaryseed commented 4 years ago

Thanks for the feedback! Based on it, I tried out a refactor that actually uses cowboy_metrics_h directly... #6

binaryseed commented 4 years ago

Which also leads me to a question.. Would you be open to a PR to cowboy that adds an init_callback to cowboy_metrics_h? That's literally all that's left in this handler.

With a callback there, we'd be able to emit the start span event, and the existing metrics_callback covers the stop & exception events...

essen commented 4 years ago

I see what you're saying, but I think your current solution works better, because it's simpler to configure. If you were using cowboy_metrics_h directly with two callbacks you'd have to configure both stream handlers and the callbacks. If you're using cowboy_telemetry_h all you need to configure is the stream handlers.

binaryseed commented 4 years ago

We're now relying on cowboy_metrics_h to gather all metrics!