apm-agent-python icon indicating copy to clipboard operation
apm-agent-python copied to clipboard

aioredis instrumentation fix with extra context and statement

Open ajaygupta2790 opened this issue 4 years ago • 7 comments

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 Screenshot 2021-11-17 at 12 14 46 AM

Adds Redis under dependencies as well

ajaygupta2790 avatar Nov 16 '21 18:11 ajaygupta2790

:green_heart: Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS Pipeline View Test View Changes Artifacts preview preview

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.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

apmmachine avatar Nov 16 '21 18:11 apmmachine

/test

ajaygupta2790 avatar Nov 16 '21 19:11 ajaygupta2790

/test

basepi avatar Nov 17 '21 17:11 basepi

Good catch! I'm not sure how this was missed in the original instrumentation.

basepi avatar Nov 17 '21 17:11 basepi

@ajaygupta2790 A few failing tests. Keep me posted if you need help resolving them.

basepi avatar Nov 18 '21 16:11 basepi

Yes I will need some help here, could you help me understand the test a little bit?

ajaygupta2790 avatar Nov 19 '21 18:11 ajaygupta2790

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.

beniwohli avatar Nov 30 '21 13:11 beniwohli

/test

basepi avatar Aug 26 '22 16:08 basepi

: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

apmmachine avatar Aug 26 '22 16:08 apmmachine

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.

basepi avatar Aug 26 '22 16:08 basepi

/test

basepi avatar Aug 26 '22 20:08 basepi

/test linters

basepi avatar Aug 26 '22 21:08 basepi

/test full

basepi avatar Aug 30 '22 18:08 basepi