-
Notifications
You must be signed in to change notification settings - Fork 469
refactor(profiling): remove unused return value #15521
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: 246 ± 3 ms. The average import time from base is: 252 ± 8 ms. The import time difference between this PR and base is: -6.7 ± 0.3 ms. Import time breakdownThe following import paths have shrunk:
|
Performance SLOsComparing candidate taegyunkim/pyframes-cleanup (9a0cad6) with baseline main (f29d01d) ❌ Test Failures (1 suite)❌ packagespackageforrootmodulemapping - 3/4❌ cache_offTime: ✅ 342.602ms (SLO: <354.300ms -3.3%) vs baseline: -1.3% Memory: ❌ 41.612MB (SLO: <41.500MB +0.3%) vs baseline: +4.0% ✅ cache_onTime: ✅ 0.384µs (SLO: <10.000µs 📉 -96.2%) vs baseline: +0.6% Memory: ✅ 40.321MB (SLO: <41.000MB 🟡 -1.7%) vs baseline: +5.2% 📈 Performance Regressions (2 suites)📈 iastaspectsospath - 24/24✅ ospathbasename_aspectTime: ✅ 5.166µs (SLO: <10.000µs 📉 -48.3%) vs baseline: 📈 +24.8% Memory: ✅ 40.226MB (SLO: <41.000MB 🟡 -1.9%) vs baseline: +4.6% ✅ ospathbasename_noaspectTime: ✅ 1.092µs (SLO: <10.000µs 📉 -89.1%) vs baseline: +0.5% Memory: ✅ 40.364MB (SLO: <41.000MB 🟡 -1.6%) vs baseline: +5.2% ✅ ospathjoin_aspectTime: ✅ 6.126µs (SLO: <10.000µs 📉 -38.7%) vs baseline: -1.0% Memory: ✅ 40.226MB (SLO: <41.000MB 🟡 -1.9%) vs baseline: +4.9% ✅ ospathjoin_noaspectTime: ✅ 2.291µs (SLO: <10.000µs 📉 -77.1%) vs baseline: -0.8% Memory: ✅ 40.088MB (SLO: <41.000MB -2.2%) vs baseline: +4.8% ✅ ospathnormcase_aspectTime: ✅ 3.416µs (SLO: <10.000µs 📉 -65.8%) vs baseline: -0.4% Memory: ✅ 40.265MB (SLO: <41.000MB 🟡 -1.8%) vs baseline: +4.9% ✅ ospathnormcase_noaspectTime: ✅ 0.566µs (SLO: <10.000µs 📉 -94.3%) vs baseline: -1.6% Memory: ✅ 40.265MB (SLO: <41.000MB 🟡 -1.8%) vs baseline: +4.6% ✅ ospathsplit_aspectTime: ✅ 4.703µs (SLO: <10.000µs 📉 -53.0%) vs baseline: -1.0% Memory: ✅ 40.187MB (SLO: <41.000MB 🟡 -2.0%) vs baseline: +4.9% ✅ ospathsplit_noaspectTime: ✅ 1.598µs (SLO: <10.000µs 📉 -84.0%) vs baseline: +0.4% Memory: ✅ 40.265MB (SLO: <41.000MB 🟡 -1.8%) vs baseline: +4.8% ✅ ospathsplitdrive_aspectTime: ✅ 3.607µs (SLO: <10.000µs 📉 -63.9%) vs baseline: -1.6% Memory: ✅ 40.206MB (SLO: <41.000MB 🟡 -1.9%) vs baseline: +4.9% ✅ ospathsplitdrive_noaspectTime: ✅ 0.699µs (SLO: <10.000µs 📉 -93.0%) vs baseline: -0.6% Memory: ✅ 40.226MB (SLO: <41.000MB 🟡 -1.9%) vs baseline: +4.7% ✅ ospathsplitext_aspectTime: ✅ 4.502µs (SLO: <10.000µs 📉 -55.0%) vs baseline: ~same Memory: ✅ 40.364MB (SLO: <41.000MB 🟡 -1.6%) vs baseline: +5.2% ✅ ospathsplitext_noaspectTime: ✅ 1.380µs (SLO: <10.000µs 📉 -86.2%) vs baseline: +0.4% Memory: ✅ 40.147MB (SLO: <41.000MB -2.1%) vs baseline: +4.9% 📈 telemetryaddmetric - 30/30✅ 1-count-metric-1-timesTime: ✅ 3.391µs (SLO: <20.000µs 📉 -83.0%) vs baseline: 📈 +16.8% Memory: ✅ 35.095MB (SLO: <35.500MB 🟡 -1.1%) vs baseline: +5.1% ✅ 1-count-metrics-100-timesTime: ✅ 200.878µs (SLO: <220.000µs -8.7%) vs baseline: -0.4% Memory: ✅ 35.095MB (SLO: <35.500MB 🟡 -1.1%) vs baseline: +4.9% ✅ 1-distribution-metric-1-timesTime: ✅ 3.283µs (SLO: <20.000µs 📉 -83.6%) vs baseline: ~same Memory: ✅ 34.859MB (SLO: <35.500MB 🟡 -1.8%) vs baseline: +4.3% ✅ 1-distribution-metrics-100-timesTime: ✅ 217.121µs (SLO: <230.000µs -5.6%) vs baseline: ~same Memory: ✅ 35.075MB (SLO: <35.500MB 🟡 -1.2%) vs baseline: +4.7% ✅ 1-gauge-metric-1-timesTime: ✅ 2.159µs (SLO: <20.000µs 📉 -89.2%) vs baseline: ~same Memory: ✅ 35.075MB (SLO: <35.500MB 🟡 -1.2%) vs baseline: +4.8% ✅ 1-gauge-metrics-100-timesTime: ✅ 136.547µs (SLO: <150.000µs -9.0%) vs baseline: -0.3% Memory: ✅ 35.173MB (SLO: <35.500MB 🟡 -0.9%) vs baseline: +5.1% ✅ 1-rate-metric-1-timesTime: ✅ 3.059µs (SLO: <20.000µs 📉 -84.7%) vs baseline: -0.2% Memory: ✅ 34.859MB (SLO: <35.500MB 🟡 -1.8%) vs baseline: +4.2% ✅ 1-rate-metrics-100-timesTime: ✅ 213.573µs (SLO: <250.000µs 📉 -14.6%) vs baseline: -0.6% Memory: ✅ 35.095MB (SLO: <35.500MB 🟡 -1.1%) vs baseline: +4.9% ✅ 100-count-metrics-100-timesTime: ✅ 20.274ms (SLO: <22.000ms -7.8%) vs baseline: -0.3% Memory: ✅ 34.741MB (SLO: <35.500MB -2.1%) vs baseline: +5.0% ✅ 100-distribution-metrics-100-timesTime: ✅ 2.270ms (SLO: <2.550ms 📉 -11.0%) vs baseline: -0.6% Memory: ✅ 35.095MB (SLO: <35.500MB 🟡 -1.1%) vs baseline: +4.8% ✅ 100-gauge-metrics-100-timesTime: ✅ 1.409ms (SLO: <1.550ms -9.1%) vs baseline: +0.8% Memory: ✅ 34.800MB (SLO: <35.500MB 🟡 -2.0%) vs baseline: +4.9% ✅ 100-rate-metrics-100-timesTime: ✅ 2.210ms (SLO: <2.550ms 📉 -13.3%) vs baseline: -0.4% Memory: ✅ 34.780MB (SLO: <35.500MB -2.0%) vs baseline: +4.9% ✅ flush-1-metricTime: ✅ 4.653µs (SLO: <20.000µs 📉 -76.7%) vs baseline: -0.3% Memory: ✅ 35.055MB (SLO: <35.500MB 🟡 -1.3%) vs baseline: +4.7% ✅ flush-100-metricsTime: ✅ 174.299µs (SLO: <250.000µs 📉 -30.3%) vs baseline: -0.3% Memory: ✅ 35.193MB (SLO: <35.500MB 🟡 -0.9%) vs baseline: +5.2% ✅ flush-1000-metricsTime: ✅ 2.178ms (SLO: <2.500ms 📉 -12.9%) vs baseline: +0.1% Memory: ✅ 35.861MB (SLO: <36.500MB 🟡 -1.7%) vs baseline: +4.5% 🟡 Near SLO Breach (15 suites)🟡 coreapiscenario - 10/10 (1 unstable)
|
| from .. import event | ||
|
|
||
| def pyframe_to_frames(frame: types.FrameType, max_nframes: int) -> typing.Tuple[typing.List[event.DDFrame], int]: ... | ||
| def pyframe_to_frames(frame: types.FrameType, max_nframes: int) -> typing.List[event.DDFrame]: ... # noqa: E302 |
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.
Why did you have to add the noqa here? E302 is explained as
Expected {expected_blank_lines:?} blank lines, found {actual_blank_lines}
Can't we just fix it?
| from __future__ import absolute_import | ||
|
|
||
| import threading | ||
| import typing |
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.
I guess this was part of the PR you already merged into main?
Description
pyframe_to_frames()returns the number of frames that are present in the original traceback, but it is no longer used.It seems like it was there to handle the case where we truncated
nnumber of frames when we were using Python exporter. Given that we switched over to libdatadog exporter, which handles frame truncation in a slightly different manner, I'll follow up with another PR for that fix.Testing
Risks
Additional Notes