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

AWS Kinesis Delivery Streams (Firehose) Entity Mapping #1300

Merged
merged 10 commits into from
Feb 5, 2025

Conversation

TimPansino
Copy link
Contributor

@TimPansino TimPansino commented Feb 3, 2025

Overview

  • Add instrumentation and tests for AWS Firehose.
  • Correct AWS Kinesis Instrumentation naming when no stream target is present.
  • Tweak boto3/botocore in tox file.
  • Fix S3Transfer test missing absolute path.

Related Github Issue

Currently depends on #1283 but will be rebased.

Copy link

github-actions bot commented Feb 3, 2025

🦙 MegaLinter status: ❌ ERROR

Descriptor Linter Files Fixed Errors Elapsed time
✅ PYTHON bandit 1 0 5.11s
✅ PYTHON black 4 0 0 1.37s
❌ PYTHON flake8 4 9 0.73s
✅ PYTHON isort 4 0 0 0.31s
✅ PYTHON pylint 4 0 5.61s

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 Feb 3, 2025
@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2025

Codecov Report

Attention: Patch coverage is 84.61538% with 4 lines in your changes missing coverage. Please review.

Project coverage is 81.29%. Comparing base (819b6d7) to head (525a287).

Files with missing lines Patch % Lines
newrelic/hooks/external_botocore.py 84.61% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1300      +/-   ##
==========================================
- Coverage   81.33%   81.29%   -0.04%     
==========================================
  Files         203      203              
  Lines       22465    22485      +20     
  Branches     3562     3565       +3     
==========================================
+ Hits        18271    18280       +9     
- Misses       3012     3019       +7     
- Partials     1182     1186       +4     

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

@TimPansino TimPansino marked this pull request as ready for review February 3, 2025 23:22
@TimPansino TimPansino requested a review from a team as a code owner February 3, 2025 23:22
@mergify mergify bot removed the merge-conflicts label Feb 3, 2025
Copy link
Contributor

@hmstepanek hmstepanek left a comment

Choose a reason for hiding this comment

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

Other than removing the message traces it looks good.

newrelic/hooks/external_botocore.py Outdated Show resolved Hide resolved
@mergify mergify bot added the tests-failing label Feb 3, 2025
@TimPansino TimPansino requested a review from hmstepanek February 4, 2025 23:04
Copy link
Contributor

@hmstepanek hmstepanek left a comment

Choose a reason for hiding this comment

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

The firehose looks good. It looks like I didn't handle those cases where the ARN can be either the data stream or the consumer. You don't have to add that here but I think we should do that. I'll leave that up to you as to when we do that (either here or in a follow up)-but in those cases where it's either-or we still need to call extract function(s)-perhaps different extract functions.

newrelic/hooks/external_botocore.py Show resolved Hide resolved
newrelic/hooks/external_botocore.py Show resolved Hide resolved
newrelic/hooks/external_botocore.py Show resolved Hide resolved
newrelic/hooks/external_botocore.py Show resolved Hide resolved
@TimPansino
Copy link
Contributor Author

The firehose looks good. It looks like I didn't handle those cases where the ARN can be either the data stream or the consumer. You don't have to add that here but I think we should do that. I'll leave that up to you as to when we do that (either here or in a follow up)-but in those cases where it's either-or we still need to call extract function(s)-perhaps different extract functions.

Let's expand on that in a follow up, so we can get the vast majority of cases covered and have it ready for demoing.

@mergify mergify bot removed the tests-failing label Feb 5, 2025
@TimPansino TimPansino merged commit de08634 into main Feb 5, 2025
56 of 57 checks passed
@TimPansino TimPansino deleted the feature-firehose branch February 5, 2025 18:42
@TimPansino TimPansino added this to the v10.6.0 milestone Feb 6, 2025
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.

3 participants