Skip to content
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

Add aioredis instrumentation #1082

Merged
merged 8 commits into from
Apr 29, 2021

Conversation

Sparkycz
Copy link
Contributor

What does this pull request do?

Adds aioredis instrumentation

Related issues

closes #1080

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 5, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 5, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 5, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'

@apmmachine
Copy link
Contributor

apmmachine commented Mar 29, 2021

💚 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

Expand to view the summary

Build stats

  • Build Cause: Started by user Benjamin Wohlwend

  • Start Time: 2021-04-29T13:28:39.399+0000

  • Duration: 22 min 23 sec

  • Commit: d941f4e

Test stats 🧪

Test Results
Failed 0
Passed 9216
Skipped 8270
Total 17486

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 9216
Skipped 8270
Total 17486

@Sparkycz
Copy link
Contributor Author

I checked the failed jobs and I think these are beyond my changes. Please, anyone, check it out and help me to solve it.

Copy link
Contributor

@beniwohli beniwohli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sparkycz awesome stuff, thanks a lot!

The original test failure was a problem on one of our Windows test machines, it went away when I restarted the test run.

The weird @houndci-bot failures seem to be related to houndci/hound#1839. Nothing we can do about that right now...

I'll do a more in-depth review later this week.

tests/instrumentation/asyncio_tests/aioredis_tests.py Outdated Show resolved Hide resolved
Co-authored-by: Benjamin Wohlwend <bw@piquadrat.ch>
Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 5, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 5, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 5, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'

@ajay1mg
Copy link

ajay1mg commented Mar 29, 2021

Thank you @Sparkycz, I checked your code and now wondering how fast you were in adding the support for this. Could you help me understand how you came up with this piece of code?

Going forward I would love to contribute to this since I may have other requirements with my APM integration

@AlexanderWert AlexanderWert added this to the 7.13 milestone Mar 31, 2021
Copy link
Contributor

@beniwohli beniwohli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sparkycz Great work! I took the liberty to push some needed boilerplate code to ensure your tests are run in our CI test matrix.

I noticed that the maintainers of aioredis are getting ready to release version 2.0.0 of the library, which from the sound of it, changes more or less completely how the API of the library works. We'll probably need to adapt some things. Would you like to do that or should I take over that part?

elasticapm/instrumentation/packages/asyncio/aioredis.py Outdated Show resolved Hide resolved
elasticapm/instrumentation/packages/asyncio/aioredis.py Outdated Show resolved Hide resolved
@Sparkycz
Copy link
Contributor Author

Sparkycz commented Apr 6, 2021

@Sparkycz Great work! I took the liberty to push some needed boilerplate code to ensure your tests are run in our CI test matrix.

I noticed that the maintainers of aioredis are getting ready to release version 2.0.0 of the library, which from the sound of it, changes more or less completely how the API of the library works. We'll probably need to adapt some things. Would you like to do that or should I take over that part?

Thanks for the boilerplate code I didn't know It's needed as well..

I'd wait for the stable release and I'll try doing that.. You imagine having two versions of the instrumentation 754731f#diff-523e55d3048a9bdd660f01803427652e28fb5a0610ad41f9c6a98575e83ef67fR22 , don't you?

@Sparkycz
Copy link
Contributor Author

Hello @beniwohli, please, what is the PR waiting for? :-)

@Sparkycz Sparkycz requested a review from beniwohli April 19, 2021 19:23
@Sparkycz
Copy link
Contributor Author

@basepi Could you unfreeze the PR please?

@beniwohli
Copy link
Contributor

@Sparkycz sorry about the wait here, we do have a lot of other work on our plate, so I didn't have a chance to merge the PR yet. I'll get it merged today and do a release soon.

@beniwohli beniwohli merged commit 7205269 into elastic:master Apr 29, 2021
@Sparkycz Sparkycz deleted the add-aioredis-instrumentation branch April 30, 2021 10:20
beniwohli added a commit to beniwohli/apm-agent-python that referenced this pull request Sep 14, 2021
* Add aioredis instrumentation

* fix code formating by black

Co-authored-by: Benjamin Wohlwend <bw@piquadrat.ch>

* add boilerplate for CI test matrix

* Use async_capture_span instead of capture_span in aioredis instrumentation

* Blacking..


Co-authored-by: Benjamin Wohlwend <beni@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Elastic support for aioredis
6 participants