feat: add additional tracing on websockets #13332
Conversation
quinna-h
changed the title
|
|
Bootstrap import analysisComparison of import times between this PR and base. SummaryThe average import time from this PR is: 276 ± 4 ms. The average import time from base is: 282 ± 5 ms. The import time difference between this PR and base is: -6.0 ± 0.2 ms. Import time breakdownThe following import paths have shrunk:
|
BenchmarksBenchmark execution time: 2025-06-04 18:10:21 Comparing candidate commit c8daade in PR branch Found 6 performance improvements and 4 performance regressions! Performance is the same for 502 metrics, 4 unstable metrics. scenario:djangosimple-profiler
scenario:djangosimple-tracer-and-profiler
scenario:flasksimple-profiler
scenario:iastdjangostartup-appsec
scenario:iastdjangostartup-iast
scenario:iastdjangostartup-tracer
scenario:startup-ddtrace_run
scenario:startup-import_ddtrace_auto
scenario:startup-import_ddtrace_auto_django
scenario:startup-import_ddtrace_auto_flask
|
| ws_span.set_metric("_dd.dm.inherited", 1) | ||
| parent_span = self.tracer.current_root_span() | ||
| if parent_span is not None: | ||
| ws_span.set_tag_str("_dd.dm.service", parent_span.service) |
There was a problem hiding this comment.
should be the handshake span's service and resource
quinna-h
force-pushed
the
quinna.halim/websockets-tracing-asgi
branch
from
47131b0 to
2cb44a0
Compare
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
| ipaddress.ip_address(client_ip) # validate ip address | ||
| span.set_tag_str("network.client.ip", client_ip) | ||
| except ValueError: | ||
| pass |
There was a problem hiding this comment.
🔴 Code Quality Violation
silent exception (...read more)
Using the pass statement in an exception block ignores the exception. Exceptions should never be ignored. Instead, the user must add code to notify an exception occurred and attempt to handle it or recover from it.
The exception to this rule is the use of StopIteration or StopAsyncIteration when implementing a custom iterator (as those errors are used to acknowledge the end of a successful iteration).
| ipaddress.ip_address(client_ip) # validate ip address | ||
| span.set_tag_str("network.client.ip", client_ip) | ||
| except ValueError: | ||
| pass |
There was a problem hiding this comment.
🔴 Code Quality Violation
silent exception (...read more)
Using the pass statement in an exception block ignores the exception. Exceptions should never be ignored. Instead, the user must add code to notify an exception occurred and attempt to handle it or recover from it.
The exception to this rule is the use of StopIteration or StopAsyncIteration when implementing a custom iterator (as those errors are used to acknowledge the end of a successful iteration).
quinna-h
changed the title
quinna-h
changed the title
| self.integration_config._websocket_messages_separate | ||
| and self.integration_config._asgi_websockets_inherit_sampling | ||
| ): | ||
| recv_span.set_metric("_dd.dm.inherited", 1) |
There was a problem hiding this comment.
There is also the need, in this case, to lock the decision maker of the recv_span to the one of the handshake. This translates in words to have the same _dd.p.dm tag. I see that's handled kind of here (
dd-trace-py/ddtrace/internal/sampling.py
Line 270 in 12a78bf
Ideally we should set the same sampling mechanism and priority also in order not to have inconsistencies. The quick thing is just to set that meta like
recv_span._meta["_dd.p.dm"] = global_root_span._meta["_dd.p.dm"]
| ipaddress.ip_address(client_ip) # validate ip address | ||
| span.set_tag_str("network.client.ip", client_ip) | ||
| except ValueError: | ||
| pass |
There was a problem hiding this comment.
🔴 Code Quality Violation
silent exception (...read more)
Using the pass statement in an exception block ignores the exception. Exceptions should never be ignored. Instead, the user must add code to notify an exception occurred and attempt to handle it or recover from it.
The exception to this rule is the use of StopIteration or StopAsyncIteration when implementing a custom iterator (as those errors are used to acknowledge the end of a successful iteration).
| "resource": "websocket /ws", | ||
| "trace_id": 2, | ||
| "span_id": 1, | ||
| "parent_id": 0, | ||
| "type": "web", | ||
| "error": 0, | ||
| "meta": { | ||
| "_dd.base_service": "tests.contrib.fastapi", | ||
| "_dd.p.dm": "-0", |
There was a problem hiding this comment.
should the fastapi.request span also have the sampling decision maker set here?
There was a problem hiding this comment.
0 means default (unset). Why it differs from the value that has the websocket.receive above? If you want to set it either it has to be manually kept in the test either you can set a rule for it
quinna-h
force-pushed
the
quinna.halim/websockets-tracing-asgi
branch
from
d02b4a1 to
ccdb2bf
Compare
|
|
||
| @pytest.mark.subprocess(env=dict(DD_TRACE_WEBSOCKET_MESSAGES_ENABLED="true")) | ||
| @snapshot(ignores=["meta._dd.span_links", "metrics.websocket.message.length"]) | ||
| # TODO: look into why one message is only 26 chars | ||
| def test_traced_websocket(test_spans, snapshot_app): |
There was a problem hiding this comment.
TODO: consolidate these next three tests using pytest.mark.parametrize
Add websocket tracing according to RFC
Checklist
Reviewer Checklist