Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add telemetry event span for socket dispatch #6019

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

danschultzer
Copy link

@danschultzer danschultzer commented Dec 16, 2024

The reason for adding this telemetry event is that my OTel spans are full of WebSocket connections that just reads GET. This is because Bandit will just use the method which is correct as the URL may be high cardinality, however the socket path is low cardinality and should show up as e.g. GET /live/websocket.

Since socket requests are handled and halted before the rest of the endpoint plugs I can't depend on Plug.Telemetry to report them and use the [:phoenix, :endpoint] events. I could depend on [:phoenix, :socket_connected] but this will only be for successful connections.

So I'm proposing that we add a [:phoenix, :socket_dispatch] event matching the [:phoenix, :router_dispatch] events. It somewhat conflicts with the existing [:phoenix, :socket_connected] though this event only have high level metadata vs socket connected having connection metadata including the result of setting up the socket.

With the ability to attach to this telemetry event we can update the :opentelemetry_phoenix to update the span name with the route template in the same way it currently does for router dispatch events.

@josevalim
Copy link
Member

Hi @danschultzer. Our long term goal is to move the socket dispatch out of being hardcoded and make it part of the regular plug pipeline. This means we would eventually "break" this event. Therefore, if we want to automate it, I would do it for the upgrade command itself, if possible, otherwise we wait.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants