Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Error Tracking Standalone Config option #30065
base: main
Are you sure you want to change the base?
Add Error Tracking Standalone Config option #30065
Changes from 11 commits
42def98
d274bb0
182e2de
8a5fddf
81aa305
d815c6c
7239492
7f535a5
ef36b54
e908828
a097126
f62c51d
7f8a8e5
ce0d656
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 this bypass may be too aggressive. The trace-agent needs to run probabilistic samplers to adapt sampling rates returned to the tracer. If this is not run, the tracer will miss updates on sampling rates.
May I get your thoughts on this @ajgajg1134 ?
edit: looking into
runSamplers()
func in this very same file, I think this may be a better place to put this logic. It already contains conditional statements on which samplers should be run (eg ProbabilisticSampler).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 may be more of a Product question but here is the reasoning:
Then I have to admit I didn't know the probabilistic sampler talked to the tracer, but I guess we wouldn't need that if the host is set as ETS and we never run it?
I discussed with @dussault-antoine this week and we concluded that it was fine to bypass all samplers but the error sampler.
Then I don't remember if I considered putting the logic in
runSamplers()
, maybe that would give better readability?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.
do we really need analyzed spans when only this error sampler is enabled? 🤔
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.
tbh I don't know a lot about analyzed spans but as it is set to
true
when running the error sampler inrunSamplers()
, I figured I would have it as wellThere 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 tag is pretty much derived from existing span properties. Can't we avoid the propagation of this tag on every span and resolve the value, if needed, in the backend? It would make the transport more efficient and less costly.
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.
We are going to need the tag for billing, there is no other way we could know they come from ETS (we don't want to bill them for APM)
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.
We have a very similar function already, maybe it's worth extending
traceContainsError()
adding a boolean parameter to determine whether exceptions should be considered?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 also works! done in ce0d656
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.
Were already iterating through events/ checking for exceptions here, may be more efficient to add
span.Meta["_dd.span_events.has_exception"] = "true"
below that line.It also seems like
Status2Error
gets called in both paths you changed, so moving to there would only require adding the logic in 1 place.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.
From what I can see, there is a check in
Status2Error
here so that it is only applied to error spans.The point being to consider non error spans that do contain exception span events in the error sampler (in addition to error spans), I don't think I can move the tag setting there unfortunately.
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, wasn't aware of this. Agreed you can't add to
Status2Error
then. May be good to still create a func to avoid duplication but is fine as is too, approving.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.
Moved into its own func in 7f8a8e5, cleaner indeed!