-
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
[serverless] Filter out spans from Lambda Library and runtime #11687
Conversation
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.
👍🏼
pkg/trace/agent/agent.go
Outdated
return | ||
} | ||
for _, chunk := range p.Chunks() { | ||
out := make([]*pb.Span, 0, len(chunk.Spans)) |
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.
Maybe we can avoid creating this array each and every time and only do it if an actual span is discarded. Alternatively, we can even use the same array and just move discarded items (by swapping them) to the last element and keeping track of the new length, slicing chunks.Spans
at the very end.
Either of the two solutions above will be much better, latter being preferred.
Since you've now added a new feature to the trace-agent, we should make sure it works as best as it can from the first go.
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.
Good idea. I see what you're saying now. For some reason I thought that this PR only affects serverless but I see it's a change to the trace agent in general which can be leveraged by other teams if they provide a discardFunction. I'm going to go ahead and implement the swap-and-resize way to remove the element as you suggested which will avoid a slice resize operation as well as avoid allocating a new struct. I'm assuming the order doesn't matter?
pkg/trace/agent/agent.go
Outdated
@@ -349,6 +355,22 @@ func newChunksArray(chunks []*pb.TraceChunk) []*pb.TraceChunk { | |||
|
|||
var _ api.StatsProcessor = (*Agent)(nil) | |||
|
|||
// filterSpans removes all spans for which the provided FilterSpan function returns true | |||
func (a *Agent) filterSpans(p *api.Payload) { |
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 should have a unit test for this individual function too, and ideally a benchmark too.
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.
Doesn't this function already have a unit test? Maybe I'm misunderstanding something.
Added the capture lambda payloads feature for universal instrumentation
29bde35
to
67f16b7
Compare
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 is better. Thanks.
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.
Thanks for your patience!
n := 0 | ||
for _, span := range chunk.Spans { | ||
if !a.DiscardSpan(span) { | ||
chunk.Spans[n] = span |
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 really like this method. I wouldn't have thought of it 🙂 👏🏻 TIL!
I think we should remove |
What does this PR do?
Motivation
Sometimes dd-trace can generate spans reflecting HTTP requests made by the Datadog Lambda Library or the Lambda runtime itself. These spans should be excluded from traces in all circumstances because they do not come from customer code. Several customers have complained about seeing these spans.
Previously, we implemented a blocklist to exclude these spans using the
ignore_resources
configuration option. This did not fully work because theresource
of the span was not always consistent (sometimes it included the URL, but sometimes it did not), and also this mechanism only applies to the root span of a trace.This PR removes the old blocklist mechanism and checks every span individually (in the Serverless agent only).
Possible Drawbacks / Trade-offs
Describe how to test/QA your changes
The included integration tests for .NET 6 show that the filtering mechanism works. To reproduce the bug, disable the filtering function and run the integration tests.
Reviewer's Checklist
Triage
milestone is set.changelog/no-changelog
label has been applied.qa/skip-qa
label is not applied.team/..
label has been applied, indicating the team(s) that should QA this change.