-
Notifications
You must be signed in to change notification settings - Fork 475
chore(httpx): revamp integration to follow repo standard #15522
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: 244 ± 2 ms. The average import time from base is: 247 ± 2 ms. The import time difference between this PR and base is: -3.09 ± 0.09 ms. Import time breakdownThe following import paths have shrunk:
|
Performance SLOsComparing candidate dubloom/revamp-httpx (0ab0e2a) with baseline main (fdd8fed) 📈 Performance Regressions (3 suites)📈 iastaspects - 118/118✅ add_aspectTime: ✅ 0.407µs (SLO: <10.000µs 📉 -95.9%) vs baseline: +0.5% Memory: ✅ 40.442MB (SLO: <41.500MB -2.5%) vs baseline: +5.2% ✅ add_inplace_aspectTime: ✅ 0.411µs (SLO: <10.000µs 📉 -95.9%) vs baseline: +1.1% Memory: ✅ 40.560MB (SLO: <41.500MB -2.3%) vs baseline: +4.8% ✅ add_inplace_noaspectTime: ✅ 0.326µs (SLO: <10.000µs 📉 -96.7%) vs baseline: +3.3% Memory: ✅ 40.390MB (SLO: <41.500MB -2.7%) vs baseline: +5.2% ✅ add_noaspectTime: ✅ 0.281µs (SLO: <10.000µs 📉 -97.2%) vs baseline: +2.0% Memory: ✅ 40.364MB (SLO: <41.500MB -2.7%) vs baseline: +5.5% ✅ bytearray_aspectTime: ✅ 1.363µs (SLO: <10.000µs 📉 -86.4%) vs baseline: +1.6% Memory: ✅ 40.265MB (SLO: <41.500MB -3.0%) vs baseline: +4.0% ✅ bytearray_extend_aspectTime: ✅ 1.501µs (SLO: <10.000µs 📉 -85.0%) vs baseline: -0.2% Memory: ✅ 40.341MB (SLO: <41.500MB -2.8%) vs baseline: +5.1% ✅ bytearray_extend_noaspectTime: ✅ 0.612µs (SLO: <10.000µs 📉 -93.9%) vs baseline: -0.3% Memory: ✅ 40.541MB (SLO: <41.500MB -2.3%) vs baseline: +5.0% ✅ bytearray_noaspectTime: ✅ 0.478µs (SLO: <10.000µs 📉 -95.2%) vs baseline: -0.7% Memory: ✅ 40.372MB (SLO: <41.500MB -2.7%) vs baseline: +5.2% ✅ bytes_aspectTime: ✅ 1.297µs (SLO: <10.000µs 📉 -87.0%) vs baseline: ~same Memory: ✅ 40.423MB (SLO: <41.500MB -2.6%) vs baseline: +5.1% ✅ bytes_noaspectTime: ✅ 0.495µs (SLO: <10.000µs 📉 -95.0%) vs baseline: ~same Memory: ✅ 40.285MB (SLO: <41.500MB -2.9%) vs baseline: +4.9% ✅ bytesio_aspectTime: ✅ 1.337µs (SLO: <10.000µs 📉 -86.6%) vs baseline: +0.4% Memory: ✅ 40.265MB (SLO: <41.500MB -3.0%) vs baseline: +4.2% ✅ bytesio_noaspectTime: ✅ 0.497µs (SLO: <10.000µs 📉 -95.0%) vs baseline: ~same Memory: ✅ 40.206MB (SLO: <41.500MB -3.1%) vs baseline: +4.6% ✅ capitalize_aspectTime: ✅ 0.731µs (SLO: <10.000µs 📉 -92.7%) vs baseline: -0.8% Memory: ✅ 40.072MB (SLO: <41.500MB -3.4%) vs baseline: +3.5% ✅ capitalize_noaspectTime: ✅ 0.434µs (SLO: <10.000µs 📉 -95.7%) vs baseline: +0.2% Memory: ✅ 40.390MB (SLO: <41.500MB -2.7%) vs baseline: +5.0% ✅ casefold_aspectTime: ✅ 0.742µs (SLO: <10.000µs 📉 -92.6%) vs baseline: +0.6% Memory: ✅ 40.226MB (SLO: <41.500MB -3.1%) vs baseline: +4.3% ✅ casefold_noaspectTime: ✅ 0.367µs (SLO: <10.000µs 📉 -96.3%) vs baseline: +0.2% Memory: ✅ 40.305MB (SLO: <41.500MB -2.9%) vs baseline: +4.7% ✅ decode_aspectTime: ✅ 0.726µs (SLO: <10.000µs 📉 -92.7%) vs baseline: +0.9% Memory: ✅ 40.343MB (SLO: <41.500MB -2.8%) vs baseline: +5.4% ✅ decode_noaspectTime: ✅ 0.418µs (SLO: <10.000µs 📉 -95.8%) vs baseline: -1.1% Memory: ✅ 40.206MB (SLO: <41.500MB -3.1%) vs baseline: +4.6% ✅ encode_aspectTime: ✅ 0.714µs (SLO: <10.000µs 📉 -92.9%) vs baseline: +1.1% Memory: ✅ 40.265MB (SLO: <41.500MB -3.0%) vs baseline: +4.7% ✅ encode_noaspectTime: ✅ 0.401µs (SLO: <10.000µs 📉 -96.0%) vs baseline: ~same Memory: ✅ 40.305MB (SLO: <41.500MB -2.9%) vs baseline: +5.0% ✅ format_aspectTime: ✅ 3.430µs (SLO: <10.000µs 📉 -65.7%) vs baseline: -0.3% Memory: ✅ 40.442MB (SLO: <41.500MB -2.5%) vs baseline: +5.5% ✅ format_map_aspectTime: ✅ 3.523µs (SLO: <10.000µs 📉 -64.8%) vs baseline: -1.4% Memory: ✅ 40.442MB (SLO: <41.500MB -2.5%) vs baseline: +4.0% ✅ format_map_noaspectTime: ✅ 0.780µs (SLO: <10.000µs 📉 -92.2%) vs baseline: +1.0% Memory: ✅ 40.338MB (SLO: <41.500MB -2.8%) vs baseline: +4.8% ✅ format_noaspectTime: ✅ 0.600µs (SLO: <10.000µs 📉 -94.0%) vs baseline: +0.4% Memory: ✅ 40.206MB (SLO: <41.500MB -3.1%) vs baseline: +4.7% ✅ index_aspectTime: ✅ 0.359µs (SLO: <10.000µs 📉 -96.4%) vs baseline: +0.9% Memory: ✅ 40.305MB (SLO: <41.500MB -2.9%) vs baseline: +4.9% ✅ index_noaspectTime: ✅ 0.282µs (SLO: <10.000µs 📉 -97.2%) vs baseline: +0.7% Memory: ✅ 40.167MB (SLO: <41.500MB -3.2%) vs baseline: +4.6% ✅ join_aspectTime: ✅ 1.336µs (SLO: <10.000µs 📉 -86.6%) vs baseline: +0.5% Memory: ✅ 40.521MB (SLO: <41.500MB -2.4%) vs baseline: +5.7% ✅ join_noaspectTime: ✅ 0.491µs (SLO: <10.000µs 📉 -95.1%) vs baseline: -0.4% Memory: ✅ 40.423MB (SLO: <41.500MB -2.6%) vs baseline: +5.2% ✅ ljust_aspectTime: ✅ 2.602µs (SLO: <20.000µs 📉 -87.0%) vs baseline: +1.4% Memory: ✅ 40.206MB (SLO: <41.500MB -3.1%) vs baseline: +3.9% ✅ ljust_noaspectTime: ✅ 0.408µs (SLO: <10.000µs 📉 -95.9%) vs baseline: +0.2% Memory: ✅ 40.226MB (SLO: <41.500MB -3.1%) vs baseline: +4.6% ✅ lower_aspectTime: ✅ 2.311µs (SLO: <10.000µs 📉 -76.9%) vs baseline: +1.4% Memory: ✅ 40.344MB (SLO: <41.500MB -2.8%) vs baseline: +4.4% ✅ lower_noaspectTime: ✅ 0.367µs (SLO: <10.000µs 📉 -96.3%) vs baseline: -0.2% Memory: ✅ 40.206MB (SLO: <41.500MB -3.1%) vs baseline: +4.7% ✅ lstrip_aspectTime: ✅ 2.255µs (SLO: <20.000µs 📉 -88.7%) vs baseline: +0.6% Memory: ✅ 40.344MB (SLO: <41.500MB -2.8%) vs baseline: +4.9% ✅ lstrip_noaspectTime: ✅ 0.384µs (SLO: <10.000µs 📉 -96.2%) vs baseline: +0.3% Memory: ✅ 40.187MB (SLO: <41.500MB -3.2%) vs baseline: +4.9% ✅ modulo_aspectTime: ✅ 1.040µs (SLO: <10.000µs 📉 -89.6%) vs baseline: +3.8% Memory: ✅ 40.247MB (SLO: <41.500MB -3.0%) vs baseline: +4.9% ✅ modulo_aspect_for_bytearray_bytearrayTime: ✅ 1.553µs (SLO: <10.000µs 📉 -84.5%) vs baseline: -0.2% Memory: ✅ 40.364MB (SLO: <41.500MB -2.7%) vs baseline: +5.0% ✅ modulo_aspect_for_bytesTime: ✅ 0.977µs (SLO: <10.000µs 📉 -90.2%) vs baseline: +0.4% Memory: ✅ 40.343MB (SLO: <41.500MB -2.8%) vs baseline: +5.3% ✅ modulo_aspect_for_bytes_bytearrayTime: ✅ 1.247µs (SLO: <10.000µs 📉 -87.5%) vs baseline: +2.6% Memory: ✅ 40.316MB (SLO: <41.500MB -2.9%) vs baseline: +4.9% ✅ modulo_noaspectTime: ✅ 0.625µs (SLO: <10.000µs 📉 -93.8%) vs baseline: -1.0% Memory: ✅ 40.128MB (SLO: <41.500MB -3.3%) vs baseline: +4.6% ✅ replace_aspectTime: ✅ 4.874µs (SLO: <10.000µs 📉 -51.3%) vs baseline: -1.1% Memory: ✅ 40.423MB (SLO: <41.500MB -2.6%) vs baseline: +4.9% ✅ replace_noaspectTime: ✅ 0.457µs (SLO: <10.000µs 📉 -95.4%) vs baseline: -1.4% Memory: ✅ 40.206MB (SLO: <41.500MB -3.1%) vs baseline: +4.4% ✅ repr_aspectTime: ✅ 0.901µs (SLO: <10.000µs 📉 -91.0%) vs baseline: -1.2% Memory: ✅ 40.501MB (SLO: <41.500MB -2.4%) vs baseline: +5.7% ✅ repr_noaspectTime: ✅ 0.417µs (SLO: <10.000µs 📉 -95.8%) vs baseline: +0.1% Memory: ✅ 40.462MB (SLO: <41.500MB -2.5%) vs baseline: +5.4% ✅ rstrip_aspectTime: ✅ 1.954µs (SLO: <20.000µs 📉 -90.2%) vs baseline: +1.9% Memory: ✅ 40.383MB (SLO: <41.500MB -2.7%) vs baseline: +5.6% ✅ rstrip_noaspectTime: ✅ 0.376µs (SLO: <10.000µs 📉 -96.2%) vs baseline: -0.7% Memory: ✅ 40.226MB (SLO: <41.500MB -3.1%) vs baseline: +4.8% ✅ slice_aspectTime: ✅ 0.488µs (SLO: <10.000µs 📉 -95.1%) vs baseline: -2.0% Memory: ✅ 40.364MB (SLO: <41.500MB -2.7%) vs baseline: +5.1% ✅ slice_noaspectTime: ✅ 0.447µs (SLO: <10.000µs 📉 -95.5%) vs baseline: -0.4% Memory: ✅ 40.206MB (SLO: <41.500MB -3.1%) vs baseline: +4.2% ✅ stringio_aspectTime: ✅ 1.764µs (SLO: <10.000µs 📉 -82.4%) vs baseline: 📈 +14.3% Memory: ✅ 40.639MB (SLO: <41.500MB -2.1%) vs baseline: +4.8% ✅ stringio_noaspectTime: ✅ 0.718µs (SLO: <10.000µs 📉 -92.8%) vs baseline: -0.2% Memory: ✅ 40.285MB (SLO: <41.500MB -2.9%) vs baseline: +4.9% ✅ strip_aspectTime: ✅ 2.235µs (SLO: <20.000µs 📉 -88.8%) vs baseline: +0.4% Memory: ✅ 40.265MB (SLO: <41.500MB -3.0%) vs baseline: +4.2% ✅ strip_noaspectTime: ✅ 0.383µs (SLO: <10.000µs 📉 -96.2%) vs baseline: +1.0% Memory: ✅ 40.383MB (SLO: <41.500MB -2.7%) vs baseline: +5.6% ✅ swapcase_aspectTime: ✅ 2.508µs (SLO: <10.000µs 📉 -74.9%) vs baseline: +3.8% Memory: ✅ 40.442MB (SLO: <41.500MB -2.5%) vs baseline: +5.0% ✅ swapcase_noaspectTime: ✅ 0.538µs (SLO: <10.000µs 📉 -94.6%) vs baseline: -0.4% Memory: ✅ 40.088MB (SLO: <41.500MB -3.4%) vs baseline: +4.2% ✅ title_aspectTime: ✅ 2.432µs (SLO: <10.000µs 📉 -75.7%) vs baseline: +2.1% Memory: ✅ 40.226MB (SLO: <41.500MB -3.1%) vs baseline: +3.9% ✅ title_noaspectTime: ✅ 0.502µs (SLO: <10.000µs 📉 -95.0%) vs baseline: +0.4% Memory: ✅ 40.167MB (SLO: <41.500MB -3.2%) vs baseline: +4.8% ✅ translate_aspectTime: ✅ 3.306µs (SLO: <10.000µs 📉 -66.9%) vs baseline: +0.4% Memory: ✅ 40.580MB (SLO: <41.500MB -2.2%) vs baseline: +5.3% ✅ translate_noaspectTime: ✅ 1.043µs (SLO: <10.000µs 📉 -89.6%) vs baseline: +0.8% Memory: ✅ 40.472MB (SLO: <41.500MB -2.5%) vs baseline: +5.6% ✅ upper_aspectTime: ✅ 2.322µs (SLO: <10.000µs 📉 -76.8%) vs baseline: +3.3% Memory: ✅ 40.108MB (SLO: <41.500MB -3.4%) vs baseline: +3.8% ✅ upper_noaspectTime: ✅ 0.368µs (SLO: <10.000µs 📉 -96.3%) vs baseline: ~same Memory: ✅ 40.187MB (SLO: <41.500MB -3.2%) vs baseline: +4.7% 📈 iastaspectsospath - 24/24✅ ospathbasename_aspectTime: ✅ 5.180µs (SLO: <10.000µs 📉 -48.2%) vs baseline: 📈 +25.4% Memory: ✅ 40.246MB (SLO: <41.000MB 🟡 -1.8%) vs baseline: +5.1% ✅ ospathbasename_noaspectTime: ✅ 1.089µs (SLO: <10.000µs 📉 -89.1%) vs baseline: +0.7% Memory: ✅ 40.265MB (SLO: <41.000MB 🟡 -1.8%) vs baseline: +5.2% ✅ ospathjoin_aspectTime: ✅ 6.176µs (SLO: <10.000µs 📉 -38.2%) vs baseline: +0.6% Memory: ✅ 40.187MB (SLO: <41.000MB 🟡 -2.0%) vs baseline: +4.8% ✅ ospathjoin_noaspectTime: ✅ 2.274µs (SLO: <10.000µs 📉 -77.3%) vs baseline: -1.3% Memory: ✅ 40.305MB (SLO: <41.000MB 🟡 -1.7%) vs baseline: +5.0% ✅ ospathnormcase_aspectTime: ✅ 3.423µs (SLO: <10.000µs 📉 -65.8%) vs baseline: +0.2% Memory: ✅ 40.108MB (SLO: <41.000MB -2.2%) vs baseline: +4.5% ✅ ospathnormcase_noaspectTime: ✅ 0.566µs (SLO: <10.000µs 📉 -94.3%) vs baseline: -1.0% Memory: ✅ 40.246MB (SLO: <41.000MB 🟡 -1.8%) vs baseline: +4.7% ✅ ospathsplit_aspectTime: ✅ 4.710µs (SLO: <10.000µs 📉 -52.9%) vs baseline: -1.0% Memory: ✅ 40.226MB (SLO: <41.000MB 🟡 -1.9%) vs baseline: +4.7% ✅ ospathsplit_noaspectTime: ✅ 1.585µs (SLO: <10.000µs 📉 -84.2%) vs baseline: -0.3% Memory: ✅ 40.285MB (SLO: <41.000MB 🟡 -1.7%) vs baseline: +4.5% ✅ ospathsplitdrive_aspectTime: ✅ 3.601µs (SLO: <10.000µs 📉 -64.0%) vs baseline: -0.5% Memory: ✅ 40.167MB (SLO: <41.000MB -2.0%) vs baseline: +4.4% ✅ ospathsplitdrive_noaspectTime: ✅ 0.692µs (SLO: <10.000µs 📉 -93.1%) vs baseline: -0.5% Memory: ✅ 40.147MB (SLO: <41.000MB -2.1%) vs baseline: +4.6% ✅ ospathsplitext_aspectTime: ✅ 4.523µs (SLO: <10.000µs 📉 -54.8%) vs baseline: ~same Memory: ✅ 40.128MB (SLO: <41.000MB -2.1%) vs baseline: +4.2% ✅ ospathsplitext_noaspectTime: ✅ 1.375µs (SLO: <10.000µs 📉 -86.3%) vs baseline: +0.2% Memory: ✅ 40.226MB (SLO: <41.000MB 🟡 -1.9%) vs baseline: +5.1% 📈 telemetryaddmetric - 30/30✅ 1-count-metric-1-timesTime: ✅ 3.407µs (SLO: <20.000µs 📉 -83.0%) vs baseline: 📈 +14.5% Memory: ✅ 34.721MB (SLO: <35.500MB -2.2%) vs baseline: +4.8% ✅ 1-count-metrics-100-timesTime: ✅ 201.857µs (SLO: <220.000µs -8.2%) vs baseline: -0.9% Memory: ✅ 34.760MB (SLO: <35.500MB -2.1%) vs baseline: +5.0% ✅ 1-distribution-metric-1-timesTime: ✅ 3.322µs (SLO: <20.000µs 📉 -83.4%) vs baseline: -1.1% Memory: ✅ 34.760MB (SLO: <35.500MB -2.1%) vs baseline: +4.3% ✅ 1-distribution-metrics-100-timesTime: ✅ 216.385µs (SLO: <230.000µs -5.9%) vs baseline: -1.7% Memory: ✅ 34.662MB (SLO: <35.500MB -2.4%) vs baseline: +4.6% ✅ 1-gauge-metric-1-timesTime: ✅ 2.189µs (SLO: <20.000µs 📉 -89.1%) vs baseline: -1.1% Memory: ✅ 34.780MB (SLO: <35.500MB -2.0%) vs baseline: +5.0% ✅ 1-gauge-metrics-100-timesTime: ✅ 136.896µs (SLO: <150.000µs -8.7%) vs baseline: ~same Memory: ✅ 34.780MB (SLO: <35.500MB -2.0%) vs baseline: +4.9% ✅ 1-rate-metric-1-timesTime: ✅ 3.093µs (SLO: <20.000µs 📉 -84.5%) vs baseline: -1.1% Memory: ✅ 34.760MB (SLO: <35.500MB -2.1%) vs baseline: +4.8% ✅ 1-rate-metrics-100-timesTime: ✅ 216.905µs (SLO: <250.000µs 📉 -13.2%) vs baseline: -0.5% Memory: ✅ 34.741MB (SLO: <35.500MB -2.1%) vs baseline: +5.1% ✅ 100-count-metrics-100-timesTime: ✅ 20.545ms (SLO: <22.000ms -6.6%) vs baseline: +1.0% Memory: ✅ 34.780MB (SLO: <35.500MB -2.0%) vs baseline: +4.7% ✅ 100-distribution-metrics-100-timesTime: ✅ 2.276ms (SLO: <2.550ms 📉 -10.8%) vs baseline: -0.6% Memory: ✅ 34.878MB (SLO: <35.500MB 🟡 -1.8%) vs baseline: +5.2% ✅ 100-gauge-metrics-100-timesTime: ✅ 1.416ms (SLO: <1.550ms -8.6%) vs baseline: +0.4% Memory: ✅ 34.701MB (SLO: <35.500MB -2.2%) vs baseline: +4.7% ✅ 100-rate-metrics-100-timesTime: ✅ 2.231ms (SLO: <2.550ms 📉 -12.5%) vs baseline: +0.9% Memory: ✅ 34.760MB (SLO: <35.500MB -2.1%) vs baseline: +4.9% ✅ flush-1-metricTime: ✅ 4.639µs (SLO: <20.000µs 📉 -76.8%) vs baseline: +0.3% Memory: ✅ 35.075MB (SLO: <35.500MB 🟡 -1.2%) vs baseline: +4.8% ✅ flush-100-metricsTime: ✅ 174.330µs (SLO: <250.000µs 📉 -30.3%) vs baseline: +0.3% Memory: ✅ 35.016MB (SLO: <35.500MB 🟡 -1.4%) vs baseline: +4.4% ✅ flush-1000-metricsTime: ✅ 2.179ms (SLO: <2.500ms 📉 -12.8%) vs baseline: -0.5% Memory: ✅ 35.901MB (SLO: <36.500MB 🟡 -1.6%) vs baseline: +4.9% 🟡 Near SLO Breach (17 suites)🟡 coreapiscenario - 10/10 (1 unstable)
|
a7ae8ef to
05a8505
Compare
05a8505 to
f7c8be6
Compare
| To configure particular ``httpx`` client instances use the :class:`Pin <ddtrace.trace.Pin>` API:: | ||
| import httpx | ||
| from ddtrace._trace.pin import Pin | ||
| client = httpx.Client() | ||
| # Override service name for this instance | ||
| Pin.override(client, service="custom-http-service") | ||
| async_client = httpx.AsyncClient( | ||
| # Override service name for this instance | ||
| Pin.override(async_client, service="custom-async-http-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.
Is there a new way to do this or have we lost this feature altogether? 🤔
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.
That's a good question, @brettlangdon @emmettbutler, do you know if we want to completely remove Pin from our integrations or we want to remove it only from the tracer retrieval part ?
Thanks in advance !
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.
do you know if we want to completely remove Pin from our integrations
The public API for Pin is fully deprecated, if someone tries to use it they will get a warning in 3.x and the imports will fail in 4.x
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.
oh, also, we 100% want to remove every single trace of Pin from our entire codebase, not just from the public API/usage.
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.
Does that mean we're removing functionality from ddtrace? It seems like users would previously have been able to set custom tags which they now can't.
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.
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.
There are lots of environment variables that can be used to set service names instead of doing so in code. DD_SERVICE, DD_SERVICE_MAPPING, DD_PEER_SERVICE_MAPPING, etc
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.
Okay – I guess since this is standard it makes sense then to remove the help info from the docstring?
|
|
||
| def _get_service_name(pin, request): | ||
| # type: (Pin, httpx.Request) -> typing.Text | ||
| def _get_service_name(request): |
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.
Seems like we've lost the type hint here – can you add it back?
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.
addressed in fe3f87b
brettlangdon
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.
some minor nits, but overall lgtm
ddtrace/_trace/trace_handlers.py
Outdated
|
|
||
| def _on_httpx_send(ctx, request): | ||
| span = ctx.span | ||
| span.set_metric(_SPAN_MEASURED_KEY, 1) |
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.
| span.set_metric(_SPAN_MEASURED_KEY, 1) | |
| span._metrics[_SPAN_MEASURED_KEY] = 1 |
more performant
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.
addressed in fe3f87b
ddtrace/_trace/trace_handlers.py
Outdated
| span.link_span(context) | ||
|
|
||
|
|
||
| def _on_httpx_send(ctx, request): |
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 should at least type the ctx, and -> None:
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 improved the overall typing of this integration and the related traced_handlers in fe3f87b
ddtrace/_trace/trace_handlers.py
Outdated
| HTTPPropagator.inject(span.context, request.headers) | ||
|
|
||
|
|
||
| def _on_httpx_send_completed(ctx, request, response, url): |
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.
same here re: 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.
addressed in fe3f87b
| service=_get_service_name(req), | ||
| tags=http_request_tags(), | ||
| ) as ctx: | ||
| core.dispatch("httpx.send", (ctx, req)) |
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 we want to avoid the extra dispatch call here, you could just pass req as an attribute into the context_with_data("httpx.request", ...) call.
then we will want to not use the generic start span handler, but at least we'd avoid triggering 2 events for this. unless we think we want to use httpx.send for something else?
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.
That was a great idea, thank you !
addressed in fe3f87b
| service=_get_service_name(req), | ||
| tags=http_request_tags(), | ||
| ) as ctx: | ||
| core.dispatch("httpx.send", (ctx, req)) |
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.
same 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.
addressed in fe3f87b
tests/contrib/httpx/test_httpx.py
Outdated
|
|
||
|
|
||
| @pytest.mark.snapshot(ignores=["meta.http.useragent"]) | ||
| def test_httpx_service_name(tracer, test_spans): |
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.
| def test_httpx_service_name(tracer, test_spans): | |
| def test_httpx_service_name(): |
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.
addressed in fe3f87b
brettlangdon
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.
some thoughts/nits but otherwise lgtm. I like this refactor
| graphql-core,graphql,3.1.7,3.2.6,True | ||
| grpcio,grpc,1.34.1,1.75.1,True | ||
| httpx,httpx,0.17.1,0.28.1,True | ||
| httpx,httpx,0.25.0,0.28.1,True |
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 probably want to add a release note for this?
technically not a breaking change, but changing the min-compatible/required version of httpx we support?
not a requirement since I am not sure if that is how we want to communicate it or not, but something to consider.
| return resp | ||
| finally: | ||
| _set_span_meta(span, req, resp) | ||
| core.dispatch("httpx.send.completed", (ctx, req, resp, _url_to_str(req.url))) |
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.
a change we can make here, similar to the context started approach
finally:
ctx.set_item("response", resp)
ctx.set_item("request.url", _url_to_str(req.url)) # althought, if we have the request, why do we need this?And then in the trace handlers, do the same approach, a custom handler for the httpx.request context ended event, and grab the response object from it.
sorry I didn't notice this pattern earlier too, but would again save on an extra dispatch call
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.
also, benefit of moving the _url_to_str to the trace handler... if no one is listening, we'll never call that function/process the req.url.
| pin = Pin.get_from(instance) | ||
| if not pin or not pin.enabled(): | ||
| return await wrapped(*args, **kwargs) | ||
| def http_request_tags() -> Dict[str, str]: |
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 are these in a function if they are static?
this is more DRY, but small performance optimization to inline them instead.
not asking for a change, probably not actually a big deal here.
|
|
||
|
|
||
| if TYPE_CHECKING: | ||
| import httpx |
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.
This import will fail in applications that type check ddtrace modules but do not install httpx (since we always load the trace handlers module).
|
|
||
| def _on_httpx_request_start(ctx: core.ExecutionContext, call_trace: bool = True, **kwargs) -> None: | ||
| span = _start_span(ctx, call_trace, **kwargs) | ||
| span._metrics[_SPAN_MEASURED_KEY] = 1 |
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.
Can we add measured as a flag in start span. It's a generic that that should be set on spans that we compute trace metrics for.
|
|
||
| request = ctx.get_item("request") | ||
|
|
||
| if trace_utils.distributed_tracing_enabled(config.httpx): |
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 think this operation is also handled by start span
| ) -> None: | ||
| span = ctx.span | ||
|
|
||
| trace_utils.set_http_meta( |
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 believe there's another handler that does something similar. We should try to consolidate handlers where we can
During my R&D week, I took httpx as an example.
Therefore, I took advantage of it to revamp httpx.
This PR brings httpx integration up to dd-trace-py standard: