-
Notifications
You must be signed in to change notification settings - Fork 411
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
refactor(di): encapsulate common signal code #11153
base: main
Are you sure you want to change the base?
Conversation
397d10d
to
ee8aa65
Compare
|
BenchmarksBenchmark execution time: 2024-10-24 15:35:54 Comparing candidate commit 1d33870 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 365 metrics, 53 unstable metrics. |
ee8aa65
to
29823fb
Compare
We encapsulate common signal boilerplate code to simplify the coding of signals and ensure homogeneous behaviour among them.
29823fb
to
1d33870
Compare
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.
Awesome stuff man!!!
I would like to see everything encapsulated under SignalContext, and remove the last few places like in debugger.py that still check for RateLimit and Condtion Mixins.
@@ -188,7 +190,9 @@ def on_span_exception( | |||
) | |||
|
|||
# Capture | |||
snapshot.line() | |||
snapshot.line(ChainMap(frame.f_locals, frame.f_globals)) |
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.
we call this ChainMap multiple locations - I think it better to have an util method e.g; createScopeFromFrame(frame) -> to avoid problems down the line if we decide to limit or remove globals...
or maybe line() should just get the frame object instead.
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.
The arguments are all different so I don't think we can make a single helper in this case 🙁
DEFAULT = "DEFAULT" | ||
ENTER = "ENTER" |
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.
scary...
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.
The RFC mentions ENTRY
as accepted value so I'm aligning with that
self._timing = ProbeEvalTiming.ENTRY | ||
|
||
probe = signal.probe | ||
if isinstance(probe, TimingMixin): |
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.
nit: would easier to read if there is else under this if with self._timing = ProbeEvalTiming.ENTRY
|
||
scope = ChainMap(signal.args, signal.frame.f_globals) | ||
if isinstance(probe, ProbeConditionMixin) and not signal._eval_condition(scope): | ||
return |
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.
if this is a bug?
we can have a probe with a condition, the condition is false. it return without setting signal.state
at the exit() - we check if signal == None - go ahead and capture.
so, probe with eval==ENTRY will capture always on exit. I think we should mark this as SKIP_COND?
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.
_eval_condition
sets the state
self.state = SignalState.SKIP_COND |
@@ -412,13 +426,28 @@ def _dd_debugger_hook(self, probe: Probe) -> None: | |||
log.error("Unsupported probe type: %r", type(probe)) | |||
return | |||
|
|||
signal.line() | |||
scope = ChainMap(actual_frame.f_locals, actual_frame.f_globals) |
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.
would love to see this also get moved under SignalContext - so there is only one place in our code that deal with those Mixin-s, what do you think?
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.
Hmm but we don't need a context for a line probe 🤔
snapshot.line() | ||
snapshot.line(ChainMap(frame.f_locals, frame.f_globals)) | ||
|
||
snapshot.state = SignalState.DONE | ||
|
||
# Collect | ||
self.__uploader__.get_collector().push(snapshot) |
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.
where does the rate-limit of exception replay happen?
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.
rate-limiting is done at the very beginning
dd-trace-py/ddtrace/debugging/_exception/replay.py
Lines 156 to 157 in 1d33870
if span.get_tag(DEBUG_INFO_TAG) == "true" or not can_capture(span): | |
# Debug info for span already captured or no budget to capture |
We encapsulate common signal boilerplate code to simplify the coding of signals and ensure homogeneous behaviour among them.
Checklist
Reviewer Checklist