Bandit: Add the request_path to the span name

[joshk]

The Phoenix instrumentation updates the span name to include the request_path, but some requests don't go through the router (like LiveView websocket connections), which means the root span is missing context for what the span is related to.

This PR changes the default Bandit span name to include the conn.request_path.

[tsloughter]

The reason it is only set when going through the router is that it can't have a high cardinality value for the span name: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-spans.md#name

If the user wants the path, no matter what, in the name then it can be set with the update_name macro in their handler.

Or if there is something about being routed to a LiveView that means it is a low cardinality value then it could be updated by the instrumentation there?

[joshk]

Hi @tsloughter , thanks for the comment and information.

I agree with you, this PR would be going against the high cardinality rule for span names.

While update_name is a great general solution, it doesn't work for sockets defined in Phoenix endpoints. eg.

socket("/live", Phoenix.LiveView.Socket, websocket: [connect_info: [session: @session_options]])

socket("/socket", NervesHubWeb.UserSocket, websocket: [connect_info: [session: @session_options]])

Is there a recommended approach to updating a parent spans name? This would work for the case of the second socket, which is in our code base, but not for the socket used for Live View communications.

[joshk]

A slightly different approach would be to update the span name only if the status code is 101.

eg. if conn.status == 101, do: Tracer.update_name("WEBSOCKET #{conn.request_path}")

This would be done in handle_request_stop/3

[joshk]

For the time being I've done this in our Phoenix app: https://github.com/nerves-hub/nerves_hub_web/pull/1612/files#diff-a90641c8be89f7c4fa30427d348f740fc6a884d91c446fdc5987b78a92dcb8e5

[tsloughter]

@joshk what about checking if the path is /live? Instead of checking for the 101.

[joshk]

@tsloughter great advice!

I checked the URL patterns for Phoenix websocket connections, and they all end with /websocket, so I changed my customization to use request_path, https://github.com/nerves-hub/nerves_hub_web/pull/1612/files#diff-a90641c8be89f7c4fa30427d348f740fc6a884d91c446fdc5987b78a92dcb8e5R16-R22

I think having this in the Phoenix instrumentation would be useful, but there is no telemetry event to hook into, which is why I ended up using the Bandit [:bandit, :request, :stop] event.

[joshk]

@tsloughter , thank you for your reviews and advice.

[tsloughter]

I think having this in the Phoenix instrumentation would be useful,

aah, wonder why Phoenix doesn't send a telemetry event for these...

[joshk]

Yeah, its a little odd and annoying, and I'm sure they would be open to a PR, but I'll delay looking into that until another day.

[danschultzer]

I'm trying to figure out if we can do something upstream to be able to attach to sockets: phoenixframework/phoenix#6019