-
Notifications
You must be signed in to change notification settings - Fork 468
fix(profiling): fix silent suppression of AssertionError exceptions in _ProfiledLock::_acquire #15089
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
Conversation
|
|
Bootstrap import analysisComparison of import times between this PR and base. SummaryThe average import time from this PR is: 241 ± 3 ms. The average import time from base is: 243 ± 3 ms. The import time difference between this PR and base is: -1.9 ± 0.1 ms. Import time breakdownThe following import paths have shrunk:
|
Performance SLOsComparing candidate vlad/lockprof-fix-raise-on-asserts (d84f2ef) with baseline main (0a50af6) 🟡 Near SLO Breach (6 suites)🟡 djangosimple - 30/30✅ appsecTime: ✅ 20.500ms (SLO: <22.300ms -8.1%) vs baseline: +0.4% Memory: ✅ 66.060MB (SLO: <67.000MB 🟡 -1.4%) vs baseline: +5.0% ✅ exception-replay-enabledTime: ✅ 1.344ms (SLO: <1.450ms -7.3%) vs baseline: +0.5% Memory: ✅ 64.049MB (SLO: <67.000MB -4.4%) vs baseline: +4.8% ✅ iastTime: ✅ 20.509ms (SLO: <22.250ms -7.8%) vs baseline: +0.3% Memory: ✅ 66.060MB (SLO: <67.000MB 🟡 -1.4%) vs baseline: +5.0% ✅ profilerTime: ✅ 15.604ms (SLO: <16.550ms -5.7%) vs baseline: +0.3% Memory: ✅ 53.866MB (SLO: <54.500MB 🟡 -1.2%) vs baseline: +4.9% ✅ resource-renamingTime: ✅ 20.514ms (SLO: <21.750ms -5.7%) vs baseline: +0.3% Memory: ✅ 66.041MB (SLO: <67.000MB 🟡 -1.4%) vs baseline: +5.0% ✅ span-code-originTime: ✅ 25.408ms (SLO: <28.200ms -9.9%) vs baseline: +1.5% Memory: ✅ 67.055MB (SLO: <69.500MB -3.5%) vs baseline: +4.5% ✅ tracerTime: ✅ 20.495ms (SLO: <21.750ms -5.8%) vs baseline: ~same Memory: ✅ 66.070MB (SLO: <67.000MB 🟡 -1.4%) vs baseline: +4.9% ✅ tracer-and-profilerTime: ✅ 22.658ms (SLO: <23.500ms -3.6%) vs baseline: -0.5% Memory: ✅ 67.358MB (SLO: <68.000MB 🟡 -0.9%) vs baseline: +4.7% ✅ tracer-dont-create-db-spansTime: ✅ 19.345ms (SLO: <21.500ms 📉 -10.0%) vs baseline: +0.3% Memory: ✅ 66.109MB (SLO: <67.000MB 🟡 -1.3%) vs baseline: +5.0% ✅ tracer-minimalTime: ✅ 16.574ms (SLO: <17.500ms -5.3%) vs baseline: -0.3% Memory: ✅ 65.755MB (SLO: <67.000MB 🟡 -1.9%) vs baseline: +4.6% ✅ tracer-nativeTime: ✅ 20.483ms (SLO: <21.750ms -5.8%) vs baseline: ~same Memory: ✅ 71.519MB (SLO: <72.500MB 🟡 -1.4%) vs baseline: +4.9% ✅ tracer-no-cachesTime: ✅ 18.486ms (SLO: <19.650ms -5.9%) vs baseline: +0.5% Memory: ✅ 66.080MB (SLO: <67.000MB 🟡 -1.4%) vs baseline: +4.9% ✅ tracer-no-databasesTime: ✅ 18.748ms (SLO: <20.100ms -6.7%) vs baseline: -0.2% Memory: ✅ 65.628MB (SLO: <67.000MB -2.0%) vs baseline: +4.5% ✅ tracer-no-middlewareTime: ✅ 20.155ms (SLO: <21.500ms -6.3%) vs baseline: -0.1% Memory: ✅ 66.100MB (SLO: <67.000MB 🟡 -1.3%) vs baseline: +5.0% ✅ tracer-no-templatesTime: ✅ 20.276ms (SLO: <22.000ms -7.8%) vs baseline: -0.1% Memory: ✅ 66.060MB (SLO: <67.000MB 🟡 -1.4%) vs baseline: +5.0% 🟡 errortrackingdjangosimple - 6/6✅ errortracking-enabled-allTime: ✅ 18.060ms (SLO: <19.850ms -9.0%) vs baseline: +0.2% Memory: ✅ 66.021MB (SLO: <66.500MB 🟡 -0.7%) vs baseline: +4.8% ✅ errortracking-enabled-userTime: ✅ 18.010ms (SLO: <19.400ms -7.2%) vs baseline: ~same Memory: ✅ 66.001MB (SLO: <66.500MB 🟡 -0.8%) vs baseline: +4.9% ✅ tracer-enabledTime: ✅ 18.354ms (SLO: <19.450ms -5.6%) vs baseline: +1.8% Memory: ✅ 65.746MB (SLO: <66.500MB 🟡 -1.1%) vs baseline: +4.9% 🟡 errortrackingflasksqli - 6/6✅ errortracking-enabled-allTime: ✅ 2.077ms (SLO: <2.300ms -9.7%) vs baseline: +0.4% Memory: ✅ 52.435MB (SLO: <53.500MB 🟡 -2.0%) vs baseline: +4.7% ✅ errortracking-enabled-userTime: ✅ 2.089ms (SLO: <2.250ms -7.1%) vs baseline: +0.7% Memory: ✅ 52.455MB (SLO: <53.500MB 🟡 -2.0%) vs baseline: +4.8% ✅ tracer-enabledTime: ✅ 2.083ms (SLO: <2.300ms -9.4%) vs baseline: +0.9% Memory: ✅ 52.298MB (SLO: <53.500MB -2.2%) vs baseline: +4.7% 🟡 flasksimple - 18/18✅ appsec-getTime: ✅ 4.595ms (SLO: <4.750ms -3.3%) vs baseline: -0.4% Memory: ✅ 62.298MB (SLO: <65.000MB -4.2%) vs baseline: +5.0% ✅ appsec-postTime: ✅ 6.619ms (SLO: <6.750ms 🟡 -1.9%) vs baseline: ~same Memory: ✅ 62.273MB (SLO: <65.000MB -4.2%) vs baseline: +5.0% ✅ appsec-telemetryTime: ✅ 4.600ms (SLO: <4.750ms -3.2%) vs baseline: -0.1% Memory: ✅ 62.164MB (SLO: <65.000MB -4.4%) vs baseline: +4.6% ✅ debuggerTime: ✅ 1.853ms (SLO: <2.000ms -7.4%) vs baseline: -0.5% Memory: ✅ 45.385MB (SLO: <47.000MB -3.4%) vs baseline: +5.5% ✅ iast-getTime: ✅ 1.867ms (SLO: <2.000ms -6.6%) vs baseline: +0.3% Memory: ✅ 42.089MB (SLO: <49.000MB 📉 -14.1%) vs baseline: +5.1% ✅ profilerTime: ✅ 1.914ms (SLO: <2.100ms -8.9%) vs baseline: -0.2% Memory: ✅ 46.543MB (SLO: <47.000MB 🟡 -1.0%) vs baseline: +5.2% ✅ resource-renamingTime: ✅ 3.366ms (SLO: <3.650ms -7.8%) vs baseline: -0.4% Memory: ✅ 52.494MB (SLO: <53.500MB 🟡 -1.9%) vs baseline: +4.9% ✅ tracerTime: ✅ 3.351ms (SLO: <3.650ms -8.2%) vs baseline: -0.4% Memory: ✅ 52.455MB (SLO: <53.500MB 🟡 -2.0%) vs baseline: +4.9% ✅ tracer-nativeTime: ✅ 3.360ms (SLO: <3.650ms -7.9%) vs baseline: ~same Memory: ✅ 58.196MB (SLO: <60.000MB -3.0%) vs baseline: +4.9% 🟡 flasksqli - 6/6✅ appsec-enabledTime: ✅ 3.958ms (SLO: <4.200ms -5.8%) vs baseline: -0.7% Memory: ✅ 62.226MB (SLO: <66.000MB -5.7%) vs baseline: +4.7% ✅ iast-enabledTime: ✅ 2.442ms (SLO: <2.800ms 📉 -12.8%) vs baseline: ~same Memory: ✅ 59.081MB (SLO: <60.000MB 🟡 -1.5%) vs baseline: +4.9% ✅ tracer-enabledTime: ✅ 2.057ms (SLO: <2.250ms -8.6%) vs baseline: +0.3% Memory: ✅ 52.494MB (SLO: <54.500MB -3.7%) vs baseline: +4.9% 🟡 telemetryaddmetric - 30/30✅ 1-count-metric-1-timesTime: ✅ 2.953µs (SLO: <20.000µs 📉 -85.2%) vs baseline: ~same Memory: ✅ 31.870MB (SLO: <34.000MB -6.3%) vs baseline: +4.8% ✅ 1-count-metrics-100-timesTime: ✅ 201.265µs (SLO: <220.000µs -8.5%) vs baseline: +1.3% Memory: ✅ 31.870MB (SLO: <34.000MB -6.3%) vs baseline: +4.9% ✅ 1-distribution-metric-1-timesTime: ✅ 3.269µs (SLO: <20.000µs 📉 -83.7%) vs baseline: ~same Memory: ✅ 31.909MB (SLO: <34.000MB -6.1%) vs baseline: +5.1% ✅ 1-distribution-metrics-100-timesTime: ✅ 213.432µs (SLO: <220.000µs -3.0%) vs baseline: +0.4% Memory: ✅ 31.890MB (SLO: <34.000MB -6.2%) vs baseline: +4.9% ✅ 1-gauge-metric-1-timesTime: ✅ 2.211µs (SLO: <20.000µs 📉 -88.9%) vs baseline: +0.6% Memory: ✅ 31.909MB (SLO: <34.000MB -6.1%) vs baseline: +4.9% ✅ 1-gauge-metrics-100-timesTime: ✅ 138.046µs (SLO: <150.000µs -8.0%) vs baseline: +0.7% Memory: ✅ 31.929MB (SLO: <34.000MB -6.1%) vs baseline: +5.1% ✅ 1-rate-metric-1-timesTime: ✅ 3.190µs (SLO: <20.000µs 📉 -84.0%) vs baseline: +3.4% Memory: ✅ 31.909MB (SLO: <34.000MB -6.1%) vs baseline: +4.8% ✅ 1-rate-metrics-100-timesTime: ✅ 213.780µs (SLO: <250.000µs 📉 -14.5%) vs baseline: +0.5% Memory: ✅ 31.909MB (SLO: <34.000MB -6.1%) vs baseline: +5.1% ✅ 100-count-metrics-100-timesTime: ✅ 20.404ms (SLO: <22.000ms -7.3%) vs baseline: +1.5% Memory: ✅ 31.890MB (SLO: <34.000MB -6.2%) vs baseline: +4.8% ✅ 100-distribution-metrics-100-timesTime: ✅ 2.281ms (SLO: <2.300ms 🟡 -0.8%) vs baseline: +2.0% Memory: ✅ 31.890MB (SLO: <34.000MB -6.2%) vs baseline: +4.9% ✅ 100-gauge-metrics-100-timesTime: ✅ 1.416ms (SLO: <1.550ms -8.6%) vs baseline: +0.5% Memory: ✅ 31.890MB (SLO: <34.000MB -6.2%) vs baseline: +4.8% ✅ 100-rate-metrics-100-timesTime: ✅ 2.216ms (SLO: <2.550ms 📉 -13.1%) vs baseline: +1.7% Memory: ✅ 31.890MB (SLO: <34.000MB -6.2%) vs baseline: +4.8% ✅ flush-1-metricTime: ✅ 4.596µs (SLO: <20.000µs 📉 -77.0%) vs baseline: -0.5% Memory: ✅ 31.909MB (SLO: <34.000MB -6.1%) vs baseline: +4.9% ✅ flush-100-metricsTime: ✅ 174.476µs (SLO: <250.000µs 📉 -30.2%) vs baseline: ~same Memory: ✅ 31.890MB (SLO: <34.000MB -6.2%) vs baseline: +4.9% ✅ flush-1000-metricsTime: ✅ 2.130ms (SLO: <2.500ms 📉 -14.8%) vs baseline: -0.3% Memory: ✅ 32.755MB (SLO: <34.500MB -5.1%) vs baseline: +5.0%
|
43e92af to
b1c180c
Compare
b1c180c to
5e752e1
Compare
5e752e1 to
12e5e54
Compare
12e5e54 to
7290ce2
Compare
061d705 to
3821df5
Compare
7ef3b6a to
a9b1eaa
Compare
|
https://gitlab.ddbuild.io/DataDog/apm-reliability/dd-trace-py/-/jobs/1207759222 Precheck is failing on |
taegyunkim
left a comment
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.
Just OOC, how did you find this behavior? were you expecting some code to raise an AssertionError but it didn't?
By reading the code :) I was trying to dedupe copy/pasted code so that it's easier to reason about it and maintain going forward (here: #15088). This was one of the things that I've realized in the process. |
a9b1eaa to
7e201dd
Compare
Fixed |
7e201dd to
ae3871e
Compare
f1a6d4a to
62fd125
Compare
62fd125 to
d84f2ef
Compare
https://datadoghq.atlassian.net/browse/PROF-12594
Description
_ProfiledLock::_maybe_update_self_nameraisesAssertionErrorin two places whenconfig.enable_asserts == True.However, they're being silently suppressed by the try/catch block in the caller,
_ProfiledLock::_acquire, method.Current estimated impact is low, as it's only ever triggered for development workflows, but still, worth making it right. This PR fixes the bug by propagating
AssertionError's by the caller.Testing
Added two new tests: one for
AssertionErrorand another for a non-AssertionErrorexception.PROD
Test fails, since the
AssertionErrorexception is not propagatedHighlighted code is for demonstration purposes only - not being merged

BRANCH
Test passes
Risks
none