aioredis instrumentation fix with extra context and statement
What does this pull request do?
aioredis instrumentation is not available in elastic apm
Just like other instrumentations like asyncpg where we show what statement is executed, this PR adds the statement that is executed using the aioredis instrumentation
An example for the same is as follows:
GET service:key

Adds Redis under dependencies as well
:green_heart: Build Succeeded
the below badges are clickable and redirect to their specific view in the CI or DOCS
![]()
![]()
![]()
![]()
![]()
Expand to view the summary
Build stats
-
Start Time: 2022-08-30T18:14:40.799+0000
-
Duration: 44 min 3 sec
Test stats :test_tube:
| Test | Results |
|---|---|
| Failed | 0 |
| Passed | 13067 |
| Skipped | 14889 |
| Total | 27956 |
:green_heart: Flaky test report
Tests succeeded.
:robot: GitHub comments
To re-run your PR in the CI, just comment with:
-
/test: Re-trigger the build. -
/test linters: Run the Python linters only. -
/test full: Run the full matrix of tests. -
/test benchmark: Run the APM Agent Python benchmarks tests. -
runelasticsearch-ci/docs: Re-trigger the docs validation. (use unformatted text in the comment!)
/test
/test
Good catch! I'm not sure how this was missed in the original instrumentation.
@ajaygupta2790 A few failing tests. Keep me posted if you need help resolving them.
Yes I will need some help here, could you help me understand the test a little bit?
Hey @ajaygupta2790
The reason that the test is failing is that you're adding a span where we didn't create spans before, namely in RedisConnectionInstrumentation. That instrumentation was added to add destination data to an already existing aioredis span, because the needed information isn't available where that span is created. It's a somewhat ugly workaround, but it works (or, used to work until we renamed the subtype in the main instrumentation to redis and didn't also make that change in the if condition in RedisConnectionInstrumentation 🤦 looks like we have a testing gap there).
More generally speaking, so far we do not capture redis queries in any of our agents. This has been a deliberate choice, as redis keys quite often contain sensitive information like user names, ids, hashed passwords and similar. The risk of leaking such data from your production system to your monitoring system (in this case, Elasticsearch), is quite high. For SQL queries, we can do some sanitization, but this isn't possible for redis keys, which are essentially free-form.
I'll take this topic to our agents team to see if we want to change this and capture redis queries going forward.
/test
:globe_with_meridians: Coverage report
| Name | Metrics % (covered/total) |
Diff |
|---|---|---|
| Packages | 100.0% (67/67) |
:green_heart: |
| Files | 100.0% (226/226) |
:green_heart: |
| Classes | 100.0% (226/226) |
:green_heart: |
| Lines | 90.565% (17700/19544) |
:+1: 0.724 |
| Conditionals | 77.584% (3243/4180) |
:+1: 0.329 |
I simplified and updated your solution to not add an additional span. Though the more I look at this the more I think it probably belongs in the instrumentation classes which actually create the spans.... I'll think on it.
/test
/test linters
/test full