Skip to content

Broken Tornado Intrumentation for Websocket #2761

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.

Sign up for GitHub

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

Jump to bottom
Open
shikanime opened this issue Jul 31, 2024 · 1 comment · May be fixed by #3498
Open

Broken Tornado Intrumentation for Websocket #2761

shikanime opened this issue Jul 31, 2024 · 1 comment · May be fixed by #3498
Labels
bug Something isn't working

Comments

@shikanime
Copy link

Describe your environment

OS: Ubuntu 22.04
Python version: 3.10.12
Package version: 0.47b0

What happened?

The WebSocketHandler.finish method, called after the WebSocket upgrade, results in lost context at https://github.com/tornadoweb/tornado/blob/1f8b2d78fee5e5a15e686815e0285d4a3c831a2c/tornado/websocket.py#L929

Steps to Reproduce

from tornado import websocket, web, ioloop

class WebsocketHandler(websocket.WebSocketHandler):
    def open(self):
        print("WebSocket opened")

    def on_message(self, message):
        self.write_message(u"You said: " + message)

    def on_close(self):
        print("WebSocket closed")


app = web.Application([
    (r'/ws', WebsocketHandler),
])

if __name__ == '__main__':
    app.listen(8888)
    ioloop.IOLoop.instance().start()
opentelemetry-instrument \
    --traces_exporter console \
    --metrics_exporter console \
    --logs_exporter console \
    --service_name cmc-front-end \
    python \
    main.py
const ws = new WebSocket("ws://localhost:8888/ws");
ws.onopen = function() {
   ws.send("Hello, world");
};
ws.onmessage = function (evt) {
    console.log(evt)
};
deno run main.js

Expected Result

Context to flow from the protocol upgrade to the message handling.

Actual Result

Context is lost after the initial phase is finished.

Additional context

No response

Would you like to implement a fix?

Yes
@shikanime shikanime added the bug Something isn't working label Jul 31, 2024
@devmonkey22 devmonkey22 mentioned this issue May 9, 2025
Open
@devmonkey22
Copy link

devmonkey22 commented May 9, 2025 •
edited
Loading

I found the same issue. Even though WebSocketHandler is a subclass of tornado's RequestHandler, its flow for the WebSocket upgrade, etc causes the normal instrumentation to not be correct. The current RequestHandler hooks are prepare, on_finish, and log_exception. However, for a WebSocketHandler, it needs to patch WebSocketHandler.on_close (or similar) instead of on_finish in order to start/record the metrics and traces properly for the websocket lifetime.

I'm not sure how people want the request/response sizes reported. I saw some similar tickets for real-time connections and how OTel should report these, but not sure if it should collect on_message data sizes or other metrics to collect data received/sent or not.

devmonkey22 added a commit to devmonkey22/opentelemetry-python-contrib that referenced this issue May 13, 2025
@devmonkey22
Add tornado WebSocketHandler instrumentation support. (open-telemetry…
82620b7 
…#2761)
@devmonkey22 devmonkey22 linked a pull request May 13, 2025 that will close this issue
Open
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add tornado WebSocketHandler instrumentation support devmonkey22/opentelemetry-python-contrib
2 participants
@devmonkey22 @shikanime