-
Notifications
You must be signed in to change notification settings - Fork 133
Hybrid agent Message Queue Traces and Transactions #1619
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 Message Queue Traces and Transactions #1619
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 #1619 +/- ##
===============================================================
+ Coverage 79.93% 80.14% +0.21%
===============================================================
Files 213 213
Lines 25168 25239 +71
Branches 4001 4023 +22
===============================================================
+ Hits 20117 20229 +112
+ Misses 3622 3579 -43
- Partials 1429 1431 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
In general implementation looks good. I think the main thing on my mind for this PR is the consumer and producer parts of the agent spec as compared to the tests and some of the comments here-they seem to be a bit at odds with each other but maybe I'm just missing something. I vaguely remember our behavior being different than some of the other agent teams in regards to when we create a trace vs a transaction for message brokers so that might be where my confusion stems from.
| with BackgroundTask(application, name="Foo"): | ||
| with tracer.start_as_current_span(name="Bar", kind=otel_api_trace.SpanKind.INTERNAL): | ||
| try: | ||
| with pytest.raises(Exception): |
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.
| with pytest.raises(Exception): | |
| with pytest.raises(Exception, match=r"Test exception message"): |
| scoped_metrics=[(f"Function/{transaction_name}", 1)], | ||
| rollup_metrics=_test_application_rollup_metrics + [(f"Function/{transaction_name}", 1)], | ||
| scoped_metrics=[(f"Function/{transaction_name} http send", 2)], | ||
| rollup_metrics=_test_application_rollup_metrics + [(f"Function/{transaction_name} http send", 2)], |
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.
Fixup for megalinter:
| rollup_metrics=_test_application_rollup_metrics + [(f"Function/{transaction_name} http send", 2)], | |
| rollup_metrics=[(f"Function/{transaction_name} http send", 2), *_test_application_rollup_metrics], |
| # While this OTel span exists it will not be explicitly | ||
| # translate to a NR trace. This may occur during the | ||
| # creation of a Transaction, which will create the root | ||
| # span. This may also occur during special cases, such |
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.
Question about this-the spec says the following:
- Creating a span with a span kind of
SpanKind.CONSUMERwill start aOtherTransaction/*transaction
and create a corresponding segment.- If the
CONSUMERspan occurs within an already existing NR transaction, it will be included in
the transaction trace.
Wouldn't the above mean that a nr trace should be created on multiple calls to the consumer?
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.
Yep! For the rest of the message queues that is what we do. For Kafka, we ended up treating the consumer as a transaction and the producer as a trace. But yes, I have another PR for the rest of the queues that tweaks this logic.
newrelic/api/opentelemetry.py
Outdated
| if not nr_trace or (nr_trace and getattr(nr_trace, "end_time", None)): | ||
| # Check to see if New Relic trace ever existed | ||
| # or, if it does, that trace has already ended | ||
| if getattr(self.nr_trace, "end_time", 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.
This isn't quite the same behavior as the previous. In the previous if the self.nr_trace was None it would also return. Do we want to match the previous behavior still?
| if getattr(self.nr_trace, "end_time", None): | |
| if not self.nr_trace or getattr(self.nr_trace, "end_time", 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.
Oof, not sure how that piece of the logic got removed--that should have stayed in there...
| # Message specific attributes | ||
| if self.attributes.get("messaging.system"): | ||
| destination_name = self.attributes.get("messaging.destination") | ||
| self.nr_transaction.destination_name = destination_name |
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.
| self.nr_transaction.destination_name = destination_name |
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 remember why I stored it as a separate variable instead of directly putting it in as a transaction attribute: I also use that name in the Kafka Nodes metric a few lines below.
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--oops missed that. Ok fine as is then!
| # NOTE: Sentinel, MessageTrace, DatastoreTrace, and | ||
| # ExternalTrace types do not have a name attribute. | ||
| self._name = name | ||
| if hasattr(self, "nr_trace") and hasattr(self.nr_trace, "name"): |
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.
It looks like we are missing coverage for this function.
newrelic/api/opentelemetry.py
Outdated
| if not nr_trace or (nr_trace and getattr(nr_trace, "end_time", None)): | ||
| # Check to see if New Relic trace ever existed | ||
| # or, if it does, that trace has already ended | ||
| if getattr(self.nr_trace, "end_time", 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.
It looks like we are missing coverage in the tests for this case.
| send_producer_message() | ||
|
|
||
| test() | ||
|
|
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.
Wondering if we should have a test here to validate that a transaction and trace are not created for a producer in the case that the span exists outside a transaction?
- Creating a span with a span kind of
SpanKind.PRODUCERwill not start a NR transaction.- If the
PRODUCERspan occurs within an already existing NR transaction, a segment will
be created and it will be included in the transaction trace.
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.
That's fair. I'll put in a test for the first scenario and the second scenario I will use an existing consumer test to prove that this is the case
Co-authored-by: Hannah Stepanek <[email protected]>
207bf9c to
c30a0e4
Compare

This PR adds support and testing for the following: