-
Notifications
You must be signed in to change notification settings - Fork 133
Hybrid agent ASGI, DB Traces, and External Traces support #1617
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
Hybrid agent ASGI, DB Traces, and External Traces support #1617
Conversation
❌MegaLinter analysis: Error
Detailed Issues❌ PYTHON / ruff - 1 errorSee detailed reports in MegaLinter artifacts |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop-hybrid-core-tracing #1617 +/- ##
===============================================================
+ Coverage 79.93% 80.14% +0.21%
===============================================================
Files 213 213
Lines 25168 25211 +43
Branches 4001 4016 +15
===============================================================
+ Hits 20117 20205 +88
+ Misses 3622 3579 -43
+ Partials 1429 1427 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| partition = "aws" | ||
| if "amazonaws.cn" in host: | ||
| partition = "aws-cn" | ||
| elif "amazonaws-us-gov.com" in host: |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High
amazonaws-us-gov.com
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 13 days ago
In general, to fix incomplete URL substring sanitization, you should parse the URL (if necessary), extract the hostname, and then perform exact or well-scoped suffix checks on that hostname instead of using arbitrary substring searches. For AWS endpoints specifically, you should compare against known, exact domain suffixes (like .amazonaws.cn and .amazonaws-us-gov.com) or match a constrained pattern, not just search for the substring anywhere.
For this code, the most direct and non-breaking fix is to (1) ensure the input is treated strictly as a host/hostname (not a full URL string), and then (2) replace the substring checks with suffix checks that correctly distinguish the AWS partitions:
- Treat
hostas the candidate hostname; if it accidentally contains a scheme or path, first parse it usingurllib.parse.urlparseand extract.hostname. - Normalize
hostto lowercase for case-insensitive comparison. - Check:
- If the hostname equals
amazonaws.cnor ends with.amazonaws.cn, treat it as theaws-cnpartition. - Else if it equals
amazonaws-us-gov.comor ends with.amazonaws-us-gov.com, treat it as theaws-us-govpartition. - Otherwise keep the default
aws.
- If the hostname equals
This avoids matching cases like evil-amazonaws-us-gov.com.example.org, because such strings do not end with the exact AWS domain suffix. To implement this, we need to import urlparse from urllib.parse, normalize host, handle the case where parsing yields no hostname, and then perform stricter endswith checks in generate_dynamodb_arn. All changes are within newrelic/core/database_utils.py, and no functionality other than safer partition detection is modified.
-
Copy modified lines R24-R25 -
Copy modified lines R917-R928
| @@ -21,6 +21,8 @@ | ||
| import re | ||
| import weakref | ||
|
|
||
| from urllib.parse import urlparse | ||
|
|
||
| from newrelic.core.config import global_settings | ||
| from newrelic.core.internal_metrics import internal_metric | ||
|
|
||
| @@ -911,11 +913,19 @@ | ||
| # There are 3 different partition options. | ||
| # See https://docs.aws.amazon.com/IAM/latest/UserGuide/reference-arns.html for details. | ||
| partition = "aws" | ||
| if "amazonaws.cn" in host: | ||
| partition = "aws-cn" | ||
| elif "amazonaws-us-gov.com" in host: | ||
| partition = "aws-us-gov" | ||
|
|
||
| # Normalize and, if necessary, parse host to extract the hostname component. | ||
| if host: | ||
| parsed = urlparse(host) | ||
| hostname = parsed.hostname or host | ||
| hostname = hostname.lower() | ||
|
|
||
| # Match against known AWS partition suffixes, not arbitrary substrings. | ||
| if hostname == "amazonaws.cn" or hostname.endswith(".amazonaws.cn"): | ||
| partition = "aws-cn" | ||
| elif hostname == "amazonaws-us-gov.com" or hostname.endswith(".amazonaws-us-gov.com"): | ||
| partition = "aws-us-gov" | ||
|
|
||
| if partition and region and account_id and target: | ||
| return f"arn:{partition}:dynamodb:{region}:{account_id:012d}:table/{target}" | ||
|
|
e909db5 to
044e9b9
Compare
hmstepanek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple suggestions (mainly about the tests)-otherwise looks good.
newrelic/api/opentelemetry.py
Outdated
| if ("exception.escaped" in self.attributes) or (self.kind in (otel_api_trace.SpanKind.SERVER, otel_api_trace.SpanKind.CONSUMER) and isinstance(current_trace(), Sentinel)): | ||
| # We need to end the transaction as well | ||
| self.nr_transaction.__exit__(*error) | ||
| self.nr_transaction.__exit__(*sys.exc_info()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you set error = sys.exc_info up above so I think this should still be *error here?
self.nr_transaction.__exit__(*error)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, you're right. Not sure why I changed it...
newrelic/api/opentelemetry.py
Outdated
| host, port = scope["server"] = tuple(scope["server"]) | ||
| else: | ||
| host, port = None, None | ||
| request_method = scope["method"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be using scope.get in case these keys don't exist?
|
|
||
| hybrid_agent_tracer_provider = TracerProvider("hybrid_agent_tracer_provider") | ||
| _TRACER_PROVIDER = hybrid_agent_tracer_provider | ||
| _TRACER_PROVIDER = TracerProvider() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just making sure we don't still need to pass "hybrid_agent_tracer_provider" into here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I changed this logic slightly to be more in line with the actual TracerProvider's functionality. I'll flesh it out some more in the future, but basically, the OTel TracerProvider doesn't really take in a name.
| app_name="Python Agent Test (Hybrid Agent, FastAPI)", default_settings=_default_settings | ||
| ) | ||
|
|
||
| os.environ["NEW_RELIC_CONFIG_FILE"] = str(Path(__file__).parent / "newrelic_fastapi.ini") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe take a look at this fixture and see if you can re-use it or do something similar instead of having these .ini files and setting env var variables/etc.
Usage example here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a fair point. I figured this would be going away soon anyway, so I didn't bother to try to clean up that logic. Basically, this will be part of the internal configuration setup and it will not be necessary to do this whole setting of the ini files.
Co-authored-by: Hannah Stepanek <[email protected]>
535788d
into
develop-hybrid-core-tracing

This PR adds support and testing for the following: