-
Notifications
You must be signed in to change notification settings - Fork 467
feat(aiokafka): add APM support for aiokafka #15111
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
|
|
Co-authored-by: Pieter Van Isacker <[email protected]>
22d3b51 to
a4bf535
Compare
Bootstrap import analysisComparison of import times between this PR and base. SummaryThe average import time from this PR is: 240 ± 2 ms. The average import time from base is: 240 ± 2 ms. The import time difference between this PR and base is: -0.29 ± 0.09 ms. Import time breakdownThe following import paths have appeared:
|
Performance SLOsComparing candidate dubloom/aiokafka-apm (3afdc42) with baseline main (48a3a00) 📈 Performance Regressions (1 suite)📈 iastaspectsospath - 24/24✅ ospathbasename_aspectTime: ✅ 4.300µs (SLO: <10.000µs 📉 -57.0%) vs baseline: +0.8% Memory: ✅ 37.395MB (SLO: <39.000MB -4.1%) vs baseline: +4.8% ✅ ospathbasename_noaspectTime: ✅ 1.088µs (SLO: <10.000µs 📉 -89.1%) vs baseline: +0.4% Memory: ✅ 37.415MB (SLO: <39.000MB -4.1%) vs baseline: +4.8% ✅ ospathjoin_aspectTime: ✅ 6.236µs (SLO: <10.000µs 📉 -37.6%) vs baseline: +1.4% Memory: ✅ 37.454MB (SLO: <39.000MB -4.0%) vs baseline: +4.9% ✅ ospathjoin_noaspectTime: ✅ 2.289µs (SLO: <10.000µs 📉 -77.1%) vs baseline: +0.2% Memory: ✅ 37.454MB (SLO: <39.000MB -4.0%) vs baseline: +4.9% ✅ ospathnormcase_aspectTime: ✅ 4.076µs (SLO: <10.000µs 📉 -59.2%) vs baseline: 📈 +15.7% Memory: ✅ 37.395MB (SLO: <39.000MB -4.1%) vs baseline: +4.9% ✅ ospathnormcase_noaspectTime: ✅ 0.573µs (SLO: <10.000µs 📉 -94.3%) vs baseline: ~same Memory: ✅ 37.415MB (SLO: <39.000MB -4.1%) vs baseline: +5.0% ✅ ospathsplit_aspectTime: ✅ 4.827µs (SLO: <10.000µs 📉 -51.7%) vs baseline: -0.8% Memory: ✅ 37.375MB (SLO: <39.000MB -4.2%) vs baseline: +4.6% ✅ ospathsplit_noaspectTime: ✅ 1.597µs (SLO: <10.000µs 📉 -84.0%) vs baseline: ~same Memory: ✅ 37.415MB (SLO: <39.000MB -4.1%) vs baseline: +4.8% ✅ ospathsplitdrive_aspectTime: ✅ 3.752µs (SLO: <10.000µs 📉 -62.5%) vs baseline: +0.5% Memory: ✅ 37.415MB (SLO: <39.000MB -4.1%) vs baseline: +4.9% ✅ ospathsplitdrive_noaspectTime: ✅ 0.696µs (SLO: <10.000µs 📉 -93.0%) vs baseline: -0.8% Memory: ✅ 37.434MB (SLO: <39.000MB -4.0%) vs baseline: +4.9% ✅ ospathsplitext_aspectTime: ✅ 4.555µs (SLO: <10.000µs 📉 -54.4%) vs baseline: -0.2% Memory: ✅ 37.473MB (SLO: <39.000MB -3.9%) vs baseline: +5.1% ✅ ospathsplitext_noaspectTime: ✅ 1.385µs (SLO: <10.000µs 📉 -86.2%) vs baseline: ~same Memory: ✅ 37.473MB (SLO: <39.000MB -3.9%) vs baseline: +4.9% 🟡 Near SLO Breach (4 suites)🟡 djangosimple - 30/30✅ appsecTime: ✅ 20.432ms (SLO: <22.300ms -8.4%) vs baseline: +0.2% Memory: ✅ 66.021MB (SLO: <67.000MB 🟡 -1.5%) vs baseline: +4.8% ✅ exception-replay-enabledTime: ✅ 1.340ms (SLO: <1.450ms -7.6%) vs baseline: ~same Memory: ✅ 64.145MB (SLO: <67.000MB -4.3%) vs baseline: +4.8% ✅ iastTime: ✅ 20.436ms (SLO: <22.250ms -8.2%) vs baseline: ~same Memory: ✅ 66.041MB (SLO: <67.000MB 🟡 -1.4%) vs baseline: +4.7% ✅ profilerTime: ✅ 15.512ms (SLO: <16.550ms -6.3%) vs baseline: -0.5% Memory: ✅ 53.809MB (SLO: <54.500MB 🟡 -1.3%) vs baseline: +4.8% ✅ resource-renamingTime: ✅ 20.535ms (SLO: <21.750ms -5.6%) vs baseline: +0.3% Memory: ✅ 66.080MB (SLO: <67.000MB 🟡 -1.4%) vs baseline: +4.9% ✅ span-code-originTime: ✅ 25.427ms (SLO: <28.200ms -9.8%) vs baseline: ~same Memory: ✅ 66.929MB (SLO: <69.500MB -3.7%) vs baseline: +4.7% ✅ tracerTime: ✅ 20.490ms (SLO: <21.750ms -5.8%) vs baseline: +0.4% Memory: ✅ 66.100MB (SLO: <67.000MB 🟡 -1.3%) vs baseline: +5.0% ✅ tracer-and-profilerTime: ✅ 22.760ms (SLO: <23.500ms -3.1%) vs baseline: +0.3% Memory: ✅ 67.559MB (SLO: <68.000MB 🟡 -0.6%) vs baseline: +5.2% ✅ tracer-dont-create-db-spansTime: ✅ 19.308ms (SLO: <21.500ms 📉 -10.2%) vs baseline: ~same Memory: ✅ 66.060MB (SLO: <67.000MB 🟡 -1.4%) vs baseline: +4.8% ✅ tracer-minimalTime: ✅ 16.579ms (SLO: <17.500ms -5.3%) vs baseline: ~same Memory: ✅ 66.041MB (SLO: <67.000MB 🟡 -1.4%) vs baseline: +5.4% ✅ tracer-nativeTime: ✅ 20.402ms (SLO: <21.750ms -6.2%) vs baseline: -0.3% Memory: ✅ 71.463MB (SLO: <72.500MB 🟡 -1.4%) vs baseline: +4.8% ✅ tracer-no-cachesTime: ✅ 18.457ms (SLO: <19.650ms -6.1%) vs baseline: ~same Memory: ✅ 66.041MB (SLO: <67.000MB 🟡 -1.4%) vs baseline: +5.1% ✅ tracer-no-databasesTime: ✅ 18.836ms (SLO: <20.100ms -6.3%) vs baseline: +0.3% Memory: ✅ 65.647MB (SLO: <67.000MB -2.0%) vs baseline: +4.8% ✅ tracer-no-middlewareTime: ✅ 20.164ms (SLO: <21.500ms -6.2%) vs baseline: ~same Memory: ✅ 66.001MB (SLO: <67.000MB 🟡 -1.5%) vs baseline: +4.7% ✅ tracer-no-templatesTime: ✅ 20.264ms (SLO: <22.000ms -7.9%) vs baseline: -0.1% Memory: ✅ 66.119MB (SLO: <67.000MB 🟡 -1.3%) vs baseline: +5.1% 🟡 errortrackingdjangosimple - 6/6✅ errortracking-enabled-allTime: ✅ 18.076ms (SLO: <19.850ms -8.9%) vs baseline: ~same Memory: ✅ 66.041MB (SLO: <66.500MB 🟡 -0.7%) vs baseline: +5.3% ✅ errortracking-enabled-userTime: ✅ 18.080ms (SLO: <19.400ms -6.8%) vs baseline: -0.1% Memory: ✅ 65.982MB (SLO: <66.500MB 🟡 -0.8%) vs baseline: +5.4% ✅ tracer-enabledTime: ✅ 18.012ms (SLO: <19.450ms -7.4%) vs baseline: -0.5% Memory: ✅ 65.647MB (SLO: <66.500MB 🟡 -1.3%) vs baseline: +4.7% 🟡 flasksimple - 18/18✅ appsec-getTime: ✅ 4.598ms (SLO: <4.750ms -3.2%) vs baseline: -0.2% Memory: ✅ 62.208MB (SLO: <65.000MB -4.3%) vs baseline: +4.6% ✅ appsec-postTime: ✅ 6.628ms (SLO: <6.750ms 🟡 -1.8%) vs baseline: ~same Memory: ✅ 62.263MB (SLO: <65.000MB -4.2%) vs baseline: +4.8% ✅ appsec-telemetryTime: ✅ 4.595ms (SLO: <4.750ms -3.3%) vs baseline: +0.3% Memory: ✅ 62.283MB (SLO: <65.000MB -4.2%) vs baseline: +4.8% ✅ debuggerTime: ✅ 1.866ms (SLO: <2.000ms -6.7%) vs baseline: +0.6% Memory: ✅ 45.124MB (SLO: <47.000MB -4.0%) vs baseline: +4.9% ✅ iast-getTime: ✅ 1.854ms (SLO: <2.000ms -7.3%) vs baseline: -0.7% Memory: ✅ 42.133MB (SLO: <49.000MB 📉 -14.0%) vs baseline: +4.7% ✅ profilerTime: ✅ 1.912ms (SLO: <2.100ms -8.9%) vs baseline: ~same Memory: ✅ 46.211MB (SLO: <47.000MB 🟡 -1.7%) vs baseline: +4.8% ✅ resource-renamingTime: ✅ 3.356ms (SLO: <3.650ms -8.0%) vs baseline: -0.3% Memory: ✅ 52.557MB (SLO: <53.500MB 🟡 -1.8%) vs baseline: +4.9% ✅ tracerTime: ✅ 3.347ms (SLO: <3.650ms -8.3%) vs baseline: ~same Memory: ✅ 52.514MB (SLO: <53.500MB 🟡 -1.8%) vs baseline: +4.7% ✅ tracer-nativeTime: ✅ 3.349ms (SLO: <3.650ms -8.3%) vs baseline: ~same Memory: ✅ 58.058MB (SLO: <60.000MB -3.2%) vs baseline: +4.9% 🟡 flasksqli - 6/6✅ appsec-enabledTime: ✅ 3.969ms (SLO: <4.200ms -5.5%) vs baseline: -0.6% Memory: ✅ 62.325MB (SLO: <66.000MB -5.6%) vs baseline: +5.0% ✅ iast-enabledTime: ✅ 2.440ms (SLO: <2.800ms 📉 -12.9%) vs baseline: -0.8% Memory: ✅ 59.041MB (SLO: <60.000MB 🟡 -1.6%) vs baseline: +4.7% ✅ tracer-enabledTime: ✅ 2.061ms (SLO: <2.250ms -8.4%) vs baseline: -0.1% Memory: ✅ 52.455MB (SLO: <54.500MB -3.8%) vs baseline: +4.6%
|
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 integration is looking really good! 👏
I have some comments about the output of a few span tags returning a different output than the other kafka integrations we have:
- kafka.partition
- kafka.partitions.*
- messaging.kafka.bootstrap.servers
- kafka.message_key
I can take another look once these comments are addressed.
| "_dd.p.tid": "69085a0800000000", | ||
| "component": "aiokafka", | ||
| "kafka.message_key": "None", | ||
| "kafka.partition": "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.
Looking at the other kafka snapshots, kafka.partition is generally an integer: https://github.com/search?q=repo%3ADataDog%2Fdd-trace-py%20%22kafka.partition%22&type=code, so I recommend updating this so it's the same.
| "kafka.topic": "getmany_multiple_messages_multiple_topics", | ||
| "language": "python", | ||
| "messaging.destination.name": "getmany_multiple_messages_multiple_topics", | ||
| "messaging.kafka.bootstrap.servers": "['127.0.0.1:29092', '127.0.0.1:29092', '127.0.0.1:29092']", |
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.
It looks like the bootstrap servers are listed three times - is this expected?
Looking at the other snapshots, it looks like it's generally one address that gets returned back: https://github.com/search?q=repo%3ADataDog%2Fdd-trace-py%20%22messaging.kafka.bootstrap.servers%22&type=code . Is this something unique to aiokafka?
| "kafka.partitions.getmany_distributed_tracing": "0", | ||
| "kafka.partitions.getmany_distributed_tracing_2": "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.
I don't see any other snapshots for kafka that result in these tags: https://github.com/search?q=repo%3ADataDog%2Fdd-trace-py+kafka.partitions&type=code.
It looks like these can have a number attached:_2. What's the goal of this span tag? Is it something that could be unbounded and lead to tons of unique span tags?
| "_dd.p.dm": "-0", | ||
| "_dd.p.tid": "69085a0500000000", | ||
| "component": "aiokafka", | ||
| "kafka.message_key": "b'test_key'", |
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.
It looks like the other kafka snapshots report this as "test_key" rather than "kafka.message_key": "b'test_key'",. Can this be updated to be consistent with the other kafka spans?
https://github.com/search?q=repo%3ADataDog%2Fdd-trace-py+kafka.message_key&type=code
Description
AIOKafkaProducer.send,AIOKafkaConsumer.getone/getmany, andcommit.DD_KAFKA_PROPAGATION_ENABLED=true.DSM support will be added in a follow-up PR.
Testing
Added snapshot tests in
test_aiokafka.pyThis is a follow up of #10068
Thanks for your initial initial contribution @ls-pieter-vanisacker