-
Notifications
You must be signed in to change notification settings - Fork 224
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
feat: outcome for spans #1968
feat: outcome for spans #1968
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪 |
Friendly request: update the description to (and ensure the commit message) references #1814 |
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.
First pass. I haven't looked at tests yet.
lib/instrumentation/span.js
Outdated
@@ -63,13 +64,16 @@ Span.prototype.customStackTrace = function (stackObj) { | |||
this._recordStackTrace(stackObj) | |||
} | |||
|
|||
Span.prototype.end = function (endTime) { | |||
Span.prototype.end = function (endTime, outcome = false) { |
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.
Is the intent to add to the Span.end()
API here? If so, why default to "false" as that isn't a valid value for "outcome".
If adding this argument, then index.d.ts needs to be updated. And then if so, I'm not sure how that works with "endTime" already being optional.
(Side note: That "endTime" is option is very non-obvious from the current code. You need to read "timer.js" carefully and see that its maybeTime
is relying on undefined >= 0 === false
. That isn't the fault of this PR, of course.)
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.
Ah, no, this is slopiness on my part. A previous version I was working on had added outcome
and a second, optional parameter. This is no longer needed -- I'll remove it.
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.
Actually -- belay what I said there. Let me revisit what early week alan was up to.
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.
Sorry for the confusion there @trent -- there was additional dead code to remove that clouded the intended design but hadn't caused the tests to fail. The current code now reflects my current thinking on this.
Specifically, no, in the end I do not want to redefine the end
API method of the span object to include an outcome. It was ultimately too awkward with the endTime
parameter already being there, but not being a part of most calls to span.end
(I'm not a fan of calls like span.end(null, 'success')
, especially for the common case)
The outcome
parameter is not needed here, or in the call to _setOutcomeFromSpanEnd
, or in the _setOutcomeFromSpanEnd
method itself.
The internal outcome property is always set from a method -- either setOutcome
or one of the _setOutcome...
methods.
The outcome is frozen when a programmer calls the public setOutcome
method. This is to ensure the value set via the public method overrides other values.
The outcome is also frozen when setting an outcome from an external/exit span ending. This is to ensure that the explicit setting to a span to unknown in the existing agent code continues to work. Freezing the span here should not interfere with any other calls to setOutcome
, as the span immediately ends.
That covers my intent -- happy to talk through any other implications this might have.
lib/instrumentation/span.js
Outdated
if (typeof statusCode !== 'undefined') { | ||
if (statusCode >= 400) { | ||
this.outcome = 'failure' | ||
this.outcome = constants.SPAN.OUTCOME.FAILURE | ||
} else { | ||
this.outcome = 'success' | ||
this.outcome = constants.SPAN.OUTCOME.SUCCESS | ||
} | ||
} | ||
|
||
this._freezeOutcome() |
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.
Does this create a case where the outcome can be frozen to "unknown" and then the subsequent span.end()
will not default the outcome to "success" in _setOutcomeFromSpanEnd()
?
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.
nod that's my reading of this preexisting code (my change only moved the existing code from strings to constants and then froze the outcome) Interestingly, it's actually this behavior that (in part) led me to add the ability to generically freeze the outcome (vs. just preferring the API set value) -- Originally if a span had an unknown
outcome and got to the end, I always changed it to be a success. However, this test failed with that behavior, so I added the freeze feature so we could freeze the outcome to unknown for this specific case.
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.
LG. Just a few recommendations.
Co-authored-by: Brandon Morelli <brandon.morelli@elastic.co>
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.
Docs LG
Implements: #1814
Spec PR: elastic/apm#299
This PR adds an API for setting the outcome value on a span or a transaction. API methods are documented, and existing on Agent, Transaction, and Span object. See docs for full usage information. This required adjusting the existing setOutcome method.
This PR also adds default agent behavior to set an outcome of
failure
on the current span when capturing an error.Checklist