-
Notifications
You must be signed in to change notification settings - Fork 133
Remove CAT #1489
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
base: develop-v12.0.0
Are you sure you want to change the base?
Remove CAT #1489
Conversation
✅MegaLinter analysis: Success
See detailed reports in MegaLinter artifacts |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-v12.0.0 #1489 +/- ##
===================================================
- Coverage 81.81% 81.03% -0.78%
===================================================
Files 207 207
Lines 23953 23697 -256
Branches 3799 3747 -52
===================================================
- Hits 19597 19203 -394
- Misses 3087 3238 +151
+ Partials 1269 1256 -13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
b718186 to
d5a2458
Compare
Good call--this is still needed for synthetics Co-authored-by: Hannah Stepanek <[email protected]>
| def _nr_process_response(response, transaction): | ||
| nr_headers = transaction.process_response(response.status, response.headers) | ||
|
|
||
| response._headers.update(nr_headers) |
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 lot of instrumentation removed from this file, but we still need it. We should be calling process_response to capture the status code for web transactions and the headers as agent attributes.
| # limitations under the License. | ||
|
|
||
| import asyncio | ||
|
|
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.
Could probably rename this file to just test_client.py now.
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.
Also, we have a test_server_dt.py but no corresponding tests for outbound DT headers in the aiohttp client. We should add tests from this, as we've just removed the test_client_cat.py file that has the only thing remotely resembling tests for this.
| @@ -52,7 +51,8 @@ def task(loop, method, exc_expected, url): | |||
| assert isinstance(text_list[0], _expected_error_class), text_list[0].__class__ | |||
| assert isinstance(text_list[1], _expected_error_class), text_list[1].__class__ | |||
| else: | |||
| assert text_list[0] == text_list[1], text_list | |||
| assert text_list[0] | |||
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.
Why was this fine? Shouldn't we be keeping the behavior of these assertions? (Context: #1489 (comment))
newrelic/hooks/framework_aiohttp.py
Outdated
|
|
||
|
|
||
| def instrument_aiohttp_web(module): | ||
| global _nr_process_response |
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.
| global _nr_process_response |
newrelic/hooks/framework_aiohttp.py
Outdated
| try: | ||
| response = await wrapped(*args, **kwargs) | ||
|
|
||
| try: | ||
| trace.process_response_headers(response.headers.items()) | ||
| trace.process_response(status_code=response.status) | ||
| except: | ||
| pass | ||
|
|
||
| return response | ||
| except Exception as e: | ||
| try: | ||
| trace.process_response_headers(e.headers.items()) | ||
| except: | ||
| pass | ||
| except Exception: | ||
| notice_error() | ||
|
|
||
| raise |
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 don't think we want to call notice_error here necessarily, it's not equivalent to what we had before. I think this outer try/except can be deleted and we end up with just the success case.
response = await wrapped(*args, **kwargs)
try:
trace.process_response(status_code=response.status)
except:
pass
return response
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.
|
|
||
| flat_event = _flatten(event) | ||
|
|
||
| assert "nr.guid" in flat_event, f"name=nr.guid, event={flat_event!r}" |
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 shouldn't be deleted
|
|
||
| apdex_perf_zone = self.apdex_perf_zone() | ||
| _add_if_not_empty("apdexPerfZone", apdex_perf_zone) | ||
| _add_if_not_empty("nr.apdexPerfZone", apdex_perf_zone) |
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.
Are we confident this isn't used in the UI anywhere?
| _process_module_definition( | ||
| "aiohttp.web_reqrep", "newrelic.hooks.framework_aiohttp", "instrument_aiohttp_web_response" | ||
| ) | ||
| _process_module_definition( | ||
| "aiohttp.web_response", "newrelic.hooks.framework_aiohttp", "instrument_aiohttp_web_response" | ||
| ) |
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.
Don't we still need these?
| assert intrinsics["error.class"] == required_attrs["error.class"] | ||
| assert intrinsics["error.message"].startswith(required_attrs["error.message"]) | ||
| assert intrinsics["error.expected"] == required_attrs["error.expected"] | ||
| assert intrinsics["nr.transactionGuid"] is not None |
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.
As far as I can tell this still exists and we need this line
| # Test default settings. | ||
|
|
||
| _expected_attributes = {"agent": TRACE_ERROR_AGENT_KEYS, "user": ERROR_USER_ATTRS, "intrinsic": ["trip_id"]} | ||
| _expected_attributes = {"agent": TRACE_ERROR_AGENT_KEYS, "user": ERROR_USER_ATTRS, "intrinsic": ["guid"]} |
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.
All of these changes where we deleted trip_id are breaking this file, it's supposed to test for the behavior of intrinsics. I replaced them all with guid and that should solve all those issues.

This PR removes Cross Application Tracing (CAT) from the agent