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

AIOBotocore instrumentation #1135

Merged
merged 15 commits into from May 23, 2024
Merged

AIOBotocore instrumentation #1135

merged 15 commits into from May 23, 2024

Conversation

lrafeei
Copy link
Contributor

@lrafeei lrafeei commented May 7, 2024

This PR provides instrumentation support for AIOBotocore

Copy link

github-actions bot commented May 7, 2024

🦙 MegaLinter status: ❌ ERROR

Descriptor Linter Files Fixed Errors Elapsed time
✅ PYTHON bandit 4 0 5.38s
✅ PYTHON black 9 0 0 2.29s
❌ PYTHON flake8 9 1 1.22s
✅ PYTHON isort 9 0 0 0.34s
❌ PYTHON pylint 9 5 7.39s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

@mergify mergify bot added the tests-failing label May 7, 2024
@lrafeei lrafeei marked this pull request as ready for review May 7, 2024 19:00
@lrafeei lrafeei requested a review from a team as a code owner May 7, 2024 19:00
@codecov-commenter
Copy link

codecov-commenter commented May 7, 2024

Codecov Report

Attention: Patch coverage is 81.25000% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 81.67%. Comparing base (20c5298) to head (b160a9c).

Files Patch % Lines
newrelic/hooks/external_aiobotocore.py 79.16% 5 Missing ⚠️
newrelic/api/web_transaction.py 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1135      +/-   ##
==========================================
- Coverage   81.71%   81.67%   -0.04%     
==========================================
  Files         192      193       +1     
  Lines       21345    21368      +23     
  Branches     3715     3716       +1     
==========================================
+ Hits        17441    17452      +11     
- Misses       2819     2831      +12     
  Partials     1085     1085              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mergify mergify bot removed the tests-failing label May 13, 2024
@lrafeei lrafeei changed the title Aiobotocore instrumentation AIOBotocore instrumentation May 14, 2024
@mergify mergify bot removed the tests-failing label May 14, 2024
@lrafeei lrafeei requested a review from TimPansino May 15, 2024 17:39
Comment on lines 34 to 39
# Because AIOBotocore's proxy functionality uses aiohttp
# and urllib3 under the hood, New Relic has portions that
# are classified as Web Transactions. This means that
# browser monitoring will now be true. However, this will
# inject unwanted JS Agent Header Fragments into SQS responses.
trace.settings.browser_monitoring.enabled = False
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this something you can do, the settings object is not a copy but just a reference to the application settings. You're permanently disabling browser monitoring everywhere in this application.

Browser monitoring shouldn't be at all relevant to an ExternalTrace anyway. The issue you're likely having is the mock server having issues with it being an aiohttp server. Why don't you try just disabling browser monitoring in conftest?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also for the record, the correct way to disable browser monitoring for an individual transaction is newrelic.agent.disable_browser_autorum()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kind of went back and forth with this. I originally had it disabled in conftest but my rationale here was that, if a customer wanted to use the proxy functionality of aiobotocore SQS, the customer would have to disable the setting themselves just because of how the proxy server in aiobotocore works. If we are OK with this, I can put something in the docs about this as well.

@lrafeei lrafeei requested a review from TimPansino May 20, 2024 17:17
@lrafeei lrafeei changed the base branch from main to develop-aiobotocore May 20, 2024 22:58
@lrafeei lrafeei changed the base branch from develop-aiobotocore to main May 22, 2024 23:03
@mergify mergify bot removed the tests-failing label May 23, 2024
@lrafeei lrafeei added this to the v9.10.0 milestone May 23, 2024
@lrafeei lrafeei merged commit b75ea43 into main May 23, 2024
50 of 51 checks passed
@lrafeei lrafeei deleted the aiobotocore-instrumentation branch May 23, 2024 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants