-
Notifications
You must be signed in to change notification settings - Fork 440
feat: add additional tracing on websockets #13332
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
base: main
Are you sure you want to change the base?
Conversation
|
Bootstrap import analysisComparison of import times between this PR and base. SummaryThe average import time from this PR is: 289 ± 6 ms. The average import time from base is: 290 ± 5 ms. The import time difference between this PR and base is: -0.9 ± 0.2 ms. Import time breakdownThe following import paths have shrunk:
|
BenchmarksBenchmark execution time: 2025-06-12 21:23:50 Comparing candidate commit cdae31c in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 463 metrics, 2 unstable metrics. scenario:iastaspectsospath-ospathsplitext_aspect
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be the handshake span's service and resource
47131b0
to
2cb44a0
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the fastapi.request span also have the sampling decision maker set here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amarziali I manually set the sampling on the root span here: https://github.com/DataDog/dd-trace-py/pull/13332/files#diff-4b6de063e968d66cb554265c5ebd1d9caeb1d876dba16014255903f50d1fabddR262
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should do in the test and not in the instrumentation code. Otherwise you will change the sampling decision.
d02b4a1
to
ccdb2bf
Compare
16d0a8b
to
dee2fd3
Compare
dee2fd3
to
ac4d82e
Compare
change to False by default
a052ecf
to
f428bcb
Compare
…kets-tracing-asgi' into quinna.halim/websockets-tracing-asgi
309f38d
to
ebd86fb
Compare
e1f8525
to
b4c016f
Compare
41fa395
to
cdae31c
Compare
Add websocket tracing according to RFC
Checklist
Reviewer Checklist