Hybrid agent set status#1599
Conversation
✅MegaLinter analysis: Success
See detailed reports in MegaLinter artifacts |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-hybrid-core-tracing #1599 +/- ##
==============================================================
Coverage ? 79.87%
==============================================================
Files ? 213
Lines ? 25087
Branches ? 3988
==============================================================
Hits ? 20039
Misses ? 3619
Partials ? 1429 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d6f4701 to
bf60a3c
Compare
bf60a3c to
2b2f909
Compare
1d76d52 to
d967a27
Compare
d967a27 to
44e9c3c
Compare
hmstepanek
left a comment
There was a problem hiding this comment.
This looks really good-I had just a couple suggestions/questions.
| return False | ||
|
|
||
| return getattr(self.nr_transaction, "priority", 1) > 0 | ||
| # If priority is either not set at this point |
There was a problem hiding this comment.
I think this is perhaps a merge conflict cause this is the same logic as the previous code except if it's None we set it to 1?
There was a problem hiding this comment.
I realized that this does something slightly different than that: the transaction.priority attribute is always going to exist, but it is initially set to None. This getattr function only returns 1 if it does not have the priority attribute at all.
So, when it is set to None and it tries to do a comparison, it fails.
There was a problem hiding this comment.
Ooooh oops-you are right!
newrelic/api/opentelemetry.py
Outdated
| since this will be either invalid or unnecessary. | ||
| """ | ||
| if isinstance(status, Status): | ||
| if (self.status and self.status.status_code is StatusCode.OK) or status.is_unset: |
There was a problem hiding this comment.
Is it even possible for self.status to be None here?
There was a problem hiding this comment.
Honestly, I couldn't think of a scenario where it would be, but I got a bit nervous about missing a use case.
| if isinstance(status, Status): | ||
| if (self.status and self.status.status_code is StatusCode.OK) or status.is_unset: | ||
| return | ||
| if description is not None: |
There was a problem hiding this comment.
Is this just a limitation of the Status type? Maybe add a comment here explaining this.
There was a problem hiding this comment.
The set_status function is a bit confusing initially; it can take in a Status (which has attributes StatusCode and an optional description) or a StatusCode. So, if just the StatusCode is passed in, we can pass in a description as well. If a Status type is passed in, it should already have the StatusCode and description in there, so passing in another description is redundant at best and conflicting at worse. So, the description being passed in this case is ignored.
| attributes.update({"exception.escaped": escaped}) | ||
| else: | ||
| self.nr_trace.notice_error(error_args, attributes=attributes) | ||
| attributes = {"exception.escaped": escaped} |
There was a problem hiding this comment.
Is this even possible that attributes is None? It would also enter this else clause if attributes was empty which seems unnecessary.
There was a problem hiding this comment.
For framework instrumentations, no. If custom instrumentation is used, it is possible not to have attributes defined when starting a span.
|
|
||
| self.set_attributes(attributes) | ||
|
|
||
| notice_error(error_args, attributes=attributes) |
There was a problem hiding this comment.
Do we need to map anything to expected or ignored params in notice error here?
There was a problem hiding this comment.
I got the impression that these were slightly different interpretations. My understanding was that
- OpenTelemetry's
expectedflag was to denote whetherrecord_exception()was called explicitly within a piece of code as opposed to being called within__end__() - New Relic's
expectedflag was something explicitly passed in through anotice_error()call within a piece of code. If theexpectedflag is not set, it then goes to the application's configuration to see if the error class or error status code are already set in a user's ignore list(s).
Based on my understanding of this, I did not think we should be passing that in under the hood.
| otel_span = trace.get_current_span() | ||
| otel_span.set_attribute("otel_span_attribute_FT", "OTel span attribute from FT") | ||
|
|
||
| @dt_enabled |
There was a problem hiding this comment.
What's going on with these dt_enabled's?
There was a problem hiding this comment.
I had neglected to put them in alongside the validate_span_events validators and it just happened to not fail until now.
| return _conditional_decorator | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
Maybe have a test that tests providing a description?
Co-authored-by: Hannah Stepanek <hstepanek@newrelic.com>

This PR adds status setting capabilities as well as adding to the
record_exceptionfunctionality.(Note that
record_exceptionis not fully complete at this point.)