-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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?
Changes from 13 commits
42def98
d274bb0
182e2de
8a5fddf
81aa305
d815c6c
7239492
7f535a5
ef36b54
e908828
a097126
f62c51d
7f8a8e5
ce0d656
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -561,8 +561,15 @@ func (a *Agent) ProcessStats(in *pb.ClientStatsPayload, lang, tracerVersion stri | |
a.ClientStatsAggregator.In <- a.processStats(in, lang, tracerVersion) | ||
} | ||
|
||
// sample performs all sampling on the processedTrace modifying it as needed and returning if the trace should be kept and the number of events in the trace | ||
// sample performs all sampling on the processedTrace modifying it as needed and returning if the trace should be kept | ||
// and the number of events in the trace | ||
func (a *Agent) sample(now time.Time, ts *info.TagStats, pt *traceutil.ProcessedTrace) (keep bool, numEvents int) { | ||
// If the agent is set for Error Tracking Standalone only the ErrorSampler is run (bypasses all other samplers). | ||
// Trace chunks that don't contain errors are dropped. | ||
if a.conf.ErrorTrackingStandalone { | ||
return a.errorSampling(now, ts, pt) | ||
} | ||
|
||
// We have a `keep` that is different from pt's `DroppedTrace` field as `DroppedTrace` will be sent to intake. | ||
// For example: We want to maintain the overall trace level sampling decision for a trace with Analytics Events | ||
// where a trace might be marked as DroppedTrace true, but we still sent analytics events in that ProcessedTrace. | ||
|
@@ -605,6 +612,48 @@ func isManualUserDrop(pt *traceutil.ProcessedTrace) bool { | |
return dm == manualSampling | ||
} | ||
|
||
// errorSampling reports trace chunks with errors and tags spans as Error Tracking Backend Standalone. | ||
// Also sets "DroppedTrace" on the chunk. | ||
func (a *Agent) errorSampling(now time.Time, ts *info.TagStats, pt *traceutil.ProcessedTrace) (keep bool, numEvents int) { | ||
sampled := a.runErrorSampler(now, *pt) | ||
numEvents = len(a.getAnalyzedEvents(pt, ts)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 |
||
if sampled { | ||
for _, span := range pt.TraceChunk.Spans { | ||
if span.Error != 0 || spanContainsExceptionSpanEvent(span) { | ||
span.Meta["_dd.error_tracking_backend_standalone.error"] = "true" | ||
} else { | ||
span.Meta["_dd.error_tracking_backend_standalone.error"] = "false" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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) |
||
} | ||
} | ||
} | ||
pt.TraceChunk.DroppedTrace = !sampled | ||
return sampled, numEvents | ||
} | ||
|
||
func spanContainsExceptionSpanEvent(span *pb.Span) bool { | ||
if hasExceptionSpanEvents, ok := span.Meta["_dd.span_events.has_exception"]; ok && hasExceptionSpanEvents == "true" { | ||
return true | ||
} | ||
return false | ||
} | ||
|
||
// runErrorSampler runs the agent's configured ErrorSampler if pt contains errors and returns the sampling decision. | ||
func (a *Agent) runErrorSampler(now time.Time, pt traceutil.ProcessedTrace) (keep bool) { | ||
if traceContainsErrorOrExceptionSpanEvent(pt.TraceChunk.Spans) { | ||
return a.ErrorsSampler.Sample(now, pt.TraceChunk.Spans, pt.Root, pt.TracerEnv) | ||
} | ||
return false | ||
} | ||
|
||
func traceContainsErrorOrExceptionSpanEvent(trace pb.Trace) bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have a very similar function already, maybe it's worth extending There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that also works! done in ce0d656 |
||
for _, span := range trace { | ||
if span.Error != 0 || spanContainsExceptionSpanEvent(span) { | ||
return true | ||
} | ||
} | ||
return false | ||
} | ||
|
||
// traceSampling reports whether the chunk should be kept as a trace, setting "DroppedTrace" on the chunk | ||
func (a *Agent) traceSampling(now time.Time, ts *info.TagStats, pt *traceutil.ProcessedTrace) (keep bool, checkAnalyticsEvents bool) { | ||
sampled, check := a.runSamplers(now, ts, *pt) | ||
|
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?