Skip to content
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

[exporter/datadog]: add ignore_resources configuration option #3245

Merged
merged 22 commits into from
Apr 30, 2021
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
eaac7fa
[exporter/datadog]: wip add blocklist for resources. wip test and doc…
ericmustin Apr 26, 2021
09b97af
[exporter/datadog]: clean up ignore resources defaults
ericmustin Apr 26, 2021
8aeb8f4
[exporter/datadog]: remove unused helpers
ericmustin Apr 26, 2021
4e0651c
[exporter/datadog]: add yaml config details
ericmustin Apr 26, 2021
b07c6bc
[exporter/datadog]: add links to datadog agent funcs
ericmustin Apr 26, 2021
112543b
[exporter/datadog]: seperate blocklist into its own file and tests
ericmustin Apr 26, 2021
bf1dd6d
Merge branch 'main' into datadog_add_ignore_resources
ericmustin Apr 26, 2021
a3f484b
[exporter/datadog]: fix copyrights
ericmustin Apr 26, 2021
0822b8f
[exporter/datadog]: pr feedback update code comments and update namin…
ericmustin Apr 27, 2021
acfe19c
Merge branch 'main' into datadog_add_ignore_resources
ericmustin Apr 27, 2021
0514abc
[exporter/datadog]: add validatioon block to config
ericmustin Apr 27, 2021
42c27bd
[exporter/datadog]: update config
ericmustin Apr 27, 2021
4582380
[exporter/datadog]: linting
ericmustin Apr 27, 2021
903aa42
[exporter/datadog]: wip add helper tests
ericmustin Apr 27, 2021
7708e57
[exporter/datadog]: merge main
ericmustin Apr 27, 2021
5905b1d
[exporter/datadog]: add helper tests
ericmustin Apr 27, 2021
a1c3979
[exporter/datadog]: linting
ericmustin Apr 27, 2021
778264a
[exporter/datadog]: merge main resolve conflicts
ericmustin Apr 28, 2021
16099f3
[exporter/datadog]: use mustcompile for denylist
ericmustin Apr 28, 2021
022d52d
[exporter/datadog]: fix tests for new config/denylist behavior
ericmustin Apr 28, 2021
7eee7cf
[exporter/datadog]: add ignore resources validation test for config
ericmustin Apr 28, 2021
08da12c
Merge branch 'main' into datadog_add_ignore_resources
ericmustin Apr 28, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions exporter/datadogexporter/blocklister.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package datadogexporter

import (
"regexp"

"github.com/DataDog/datadog-agent/pkg/trace/exportable/pb"
)

// From: https://github.com/DataDog/datadog-agent/blob/a6872e436681ea2136cf8a67465e99fdb4450519/pkg/trace/filters/blacklister.go#L15-L19
ericmustin marked this conversation as resolved.
Show resolved Hide resolved
// Blocklister holds a list of regular expressions which will match resources
// on spans that should be dropped.
type Blocklister struct {
ericmustin marked this conversation as resolved.
Show resolved Hide resolved
list []*regexp.Regexp
}

// From: https://github.com/DataDog/datadog-agent/blob/a6872e436681ea2136cf8a67465e99fdb4450519/pkg/trace/filters/blacklister.go#L21-L29
// Allows returns true if the Blocklister permits this span.
func (f *Blocklister) Allows(span *pb.Span) bool {
for _, entry := range f.list {
if entry.MatchString(span.Resource) {
return false
}
}
return true
}

// From: https://github.com/DataDog/datadog-agent/blob/a6872e436681ea2136cf8a67465e99fdb4450519/pkg/trace/filters/blacklister.go#L41-L45
// NewBlocklister creates a new Blocklister based on the given list of
// regular expressions.
func NewBlocklister(exprs []string) *Blocklister {
return &Blocklister{list: compileRules(exprs)}
}

// From: https://github.com/DataDog/datadog-agent/blob/a6872e436681ea2136cf8a67465e99fdb4450519/pkg/trace/filters/blacklister.go#L47-L59
// compileRules compiles as many rules as possible from the list of expressions.
func compileRules(exprs []string) []*regexp.Regexp {
list := make([]*regexp.Regexp, 0, len(exprs))
for _, entry := range exprs {
rule, err := regexp.Compile(entry)
if err != nil {
// log.Errorf("Invalid resource filter: %q", entry)
continue
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should handle this instead of just ignoring it. I think we can

  1. On the configuration, validate the regexes using regexp.Compile. Following add validatable interface to all the component config opentelemetry-collector#2898 we should do this on a Validate method from the configuration.
  2. Use regexp.MustCompile and ensure that we only pass valid regexes to the NewBlocklister.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems fine to me but I'm a bit confused on the behavior you want here.

In the Validate method should i just return an error if invalid, or should i be sanitizing the values from a slice of strings into a slice of regexs?

Where am I using MustCompile ? I don't quite follow this part.

anyway i pushed up an attempt at this but not sure how mustcompile fits in

list = append(list, rule)
}
return list
}
85 changes: 85 additions & 0 deletions exporter/datadogexporter/blocklister_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package datadogexporter

import (
"testing"

"github.com/DataDog/datadog-agent/pkg/trace/exportable/pb"
"github.com/stretchr/testify/assert"
)

// https://github.com/DataDog/datadog-agent/blob/d3a4cd66314d70162e2c76d2481f4b93455cf717/pkg/trace/test/testutil/span.go#L372
// TestSpan returns a fix span with hardcoded info, useful for reproducible tests
func testSpan() *pb.Span {
return &pb.Span{
Duration: 10000000,
Error: 0,
Resource: "GET /some/raclette",
Service: "django",
Name: "django.controller",
SpanID: 42,
Start: 1472732573337575936,
TraceID: 424242,
Meta: map[string]string{
"user": "eric",
"pool": "fondue",
},
Metrics: map[string]float64{
"cheese_weight": 100000.0,
},
ParentID: 1111,
Type: "http",
}
}

func TestBlocklister(t *testing.T) {
tests := []struct {
filter []string
resource string
expectation bool
}{
{[]string{"/foo/bar"}, "/foo/bar", false},
{[]string{"/foo/b.r"}, "/foo/bar", false},
{[]string{"[0-9]+"}, "/abcde", true},
{[]string{"[0-9]+"}, "/abcde123", false},
{[]string{"\\(foobar\\)"}, "(foobar)", false},
{[]string{"\\(foobar\\)"}, "(bar)", true},
{[]string{"(GET|POST) /healthcheck"}, "GET /foobar", true},
{[]string{"(GET|POST) /healthcheck"}, "GET /healthcheck", false},
{[]string{"(GET|POST) /healthcheck"}, "POST /healthcheck", false},
{[]string{"SELECT COUNT\\(\\*\\) FROM BAR"}, "SELECT COUNT(*) FROM BAR", false},
{[]string{"[123"}, "[123", true},
{[]string{"\\[123"}, "[123", false},
{[]string{"ABC+", "W+"}, "ABCCCC", false},
{[]string{"ABC+", "W+"}, "WWW", false},
}

for _, test := range tests {
span := testSpan()
span.Resource = test.resource
filter := NewBlocklister(test.filter)

assert.Equal(t, test.expectation, filter.Allows(span))
}
}

func TestCompileRules(t *testing.T) {
filter := NewBlocklister([]string{"[123", "]123", "{6}"})
for i := 0; i < 100; i++ {
span := testSpan()
assert.True(t, filter.Allows(span))
}
}
10 changes: 10 additions & 0 deletions exporter/datadogexporter/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,12 @@ type TracesConfig struct {
// meaning no sampling. If you want to send one event out of every 250
// times Send() is called, you would specify 250 here.
SampleRate uint `mapstructure:"sample_rate"`

// ignored resources
// A blacklist of regular expressions can be provided to disable certain traces based on their resource name
// all entries must be surrounded by double quotes and separated by commas.
// ignore_resources: ["(GET|POST) /healthcheck"]
IgnoreResources []string `mapstructure:"ignore_resources"`
}

// TagsConfig defines the tag-related configuration
Expand Down Expand Up @@ -224,5 +230,9 @@ func (c *Config) Sanitize() error {
c.Traces.TCPAddr.Endpoint = fmt.Sprintf("https://trace.agent.%s", c.API.Site)
}

if c.Traces.IgnoreResources == nil {
c.Traces.IgnoreResources = []string{}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nil slice works pretty much like an empty slice, I think we can remove this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

return nil
}
6 changes: 6 additions & 0 deletions exporter/datadogexporter/example/config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,12 @@ exporters:
#
# endpoint: https://api.datadoghq.com

## @param ignore_resources - list of strings - optional
## A blacklist of regular expressions can be provided to disable certain traces based on their resource name
## all entries must be surrounded by double quotes and separated by commas.
#
# ignore_resources: ["(GET|POST) /healthcheck"]

service:
pipelines:
traces:
Expand Down
1 change: 1 addition & 0 deletions exporter/datadogexporter/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ func createDefaultConfig() config.Exporter {
TCPAddr: confignet.TCPAddr{
Endpoint: "$DD_APM_URL", // If not provided, set during config sanitization
},
IgnoreResources: []string{},
},

SendMetadata: true,
Expand Down
5 changes: 5 additions & 0 deletions exporter/datadogexporter/factory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ func TestCreateDefaultConfig(t *testing.T) {
TCPAddr: confignet.TCPAddr{
Endpoint: "$DD_APM_URL",
},
IgnoreResources: []string{},
},

TagsConfig: ddconfig.TagsConfig{
Expand Down Expand Up @@ -131,6 +132,7 @@ func TestLoadConfig(t *testing.T) {
TCPAddr: confignet.TCPAddr{
Endpoint: "https://trace.agent.datadoghq.eu",
},
IgnoreResources: []string{},
},
SendMetadata: true,
OnlyMetadata: false,
Expand Down Expand Up @@ -173,6 +175,7 @@ func TestLoadConfig(t *testing.T) {
TCPAddr: confignet.TCPAddr{
Endpoint: "https://trace.agent.datadoghq.com",
},
IgnoreResources: []string{},
},
SendMetadata: true,
OnlyMetadata: false,
Expand Down Expand Up @@ -257,6 +260,7 @@ func TestLoadConfigEnvVariables(t *testing.T) {
TCPAddr: confignet.TCPAddr{
Endpoint: "https://trace.agent.datadoghq.test",
},
IgnoreResources: []string{},
},
SendMetadata: true,
OnlyMetadata: false,
Expand Down Expand Up @@ -302,6 +306,7 @@ func TestLoadConfigEnvVariables(t *testing.T) {
TCPAddr: confignet.TCPAddr{
Endpoint: "https://trace.agent.datadoghq.com",
},
IgnoreResources: []string{},
},
SendMetadata: true,
OnlyMetadata: false,
Expand Down
7 changes: 6 additions & 1 deletion exporter/datadogexporter/traces_exporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ type traceExporter struct {
obfuscator *obfuscate.Obfuscator
calculator *sublayerCalculator
client *datadog.Client
blocklister *Blocklister
}

var (
Expand Down Expand Up @@ -85,6 +86,9 @@ func newTracesExporter(ctx context.Context, params component.ExporterCreateParam
// https://github.com/DataDog/datadog-serverless-functions/blob/11f170eac105d66be30f18eda09eca791bc0d31b/aws/logs_monitoring/trace_forwarder/cmd/trace/main.go#L43
obfuscator := obfuscate.NewObfuscator(obfuscatorConfig)

// a blocklist for dropping ignored resources
blocklister := NewBlocklister(cfg.Traces.IgnoreResources)

calculator := &sublayerCalculator{sc: stats.NewSublayerCalculator()}
exporter := &traceExporter{
params: params,
Expand All @@ -94,6 +98,7 @@ func newTracesExporter(ctx context.Context, params component.ExporterCreateParam
obfuscator: obfuscator,
calculator: calculator,
client: client,
blocklister: blocklister,
}

return exporter
Expand Down Expand Up @@ -131,7 +136,7 @@ func (exp *traceExporter) pushTraceData(
// convert traces to datadog traces and group trace payloads by env
// we largely apply the same logic as the serverless implementation, simplified a bit
// https://github.com/DataDog/datadog-serverless-functions/blob/f5c3aedfec5ba223b11b76a4239fcbf35ec7d045/aws/logs_monitoring/trace_forwarder/cmd/trace/main.go#L61-L83
ddTraces, ms := convertToDatadogTd(td, exp.calculator, exp.cfg)
ddTraces, ms := convertToDatadogTd(td, exp.calculator, exp.cfg, exp.blocklister)

// group the traces by env to reduce the number of flushes
aggregatedTraces := aggregateTracePayloadsByEnv(ddTraces)
Expand Down
1 change: 1 addition & 0 deletions exporter/datadogexporter/traces_exporter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ func testTracesExporterHelper(td pdata.Traces, t *testing.T) []string {
TCPAddr: confignet.TCPAddr{
Endpoint: server.URL,
},
IgnoreResources: []string{},
},
}

Expand Down
34 changes: 29 additions & 5 deletions exporter/datadogexporter/translate_traces.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ const (
)

// converts Traces into an array of datadog trace payloads grouped by env
func convertToDatadogTd(td pdata.Traces, calculator *sublayerCalculator, cfg *config.Config) ([]*pb.TracePayload, []datadog.Metric) {
func convertToDatadogTd(td pdata.Traces, calculator *sublayerCalculator, cfg *config.Config, blk *Blocklister) ([]*pb.TracePayload, []datadog.Metric) {
// TODO:
// do we apply other global tags, like version+service, to every span or only root spans of a service
// should globalTags['service'] take precedence over a trace's resource.service.name? I don't believe so, need to confirm
Expand All @@ -77,7 +77,7 @@ func convertToDatadogTd(td pdata.Traces, calculator *sublayerCalculator, cfg *co
hostname = resHostname
}

payload := resourceSpansToDatadogSpans(rs, calculator, hostname, cfg)
payload := resourceSpansToDatadogSpans(rs, calculator, hostname, cfg, blk)
traces = append(traces, &payload)

ms := metrics.DefaultMetrics("traces", uint64(pushTime))
Expand Down Expand Up @@ -115,7 +115,7 @@ func aggregateTracePayloadsByEnv(tracePayloads []*pb.TracePayload) []*pb.TracePa
}

// converts a Trace's resource spans into a trace payload
func resourceSpansToDatadogSpans(rs pdata.ResourceSpans, calculator *sublayerCalculator, hostname string, cfg *config.Config) pb.TracePayload {
func resourceSpansToDatadogSpans(rs pdata.ResourceSpans, calculator *sublayerCalculator, hostname string, cfg *config.Config, blk *Blocklister) pb.TracePayload {
// get env tag
env := cfg.Env

Expand Down Expand Up @@ -168,7 +168,29 @@ func resourceSpansToDatadogSpans(rs pdata.ResourceSpans, calculator *sublayerCal
}
}

for _, apiTrace := range apiTraces {
for traceID, apiTrace := range apiTraces {
// first drop trace if root span exists in trace chunk that is on blocklist (drop trace no stats).
// In the dd-agent, the blocklist/blacklist behavior can be performed before most of the expensive
// operations on the span.
// See: https://github.com/DataDog/datadog-agent/blob/a6872e436681ea2136cf8a67465e99fdb4450519/pkg/trace/agent/agent.go#L200
// However, in our case, the span must be converted from otlp span into a dd span first. This is for 2 reasons.
// First, DD trace chunks rec'd by datadog-agent v0.4+ endpoint are expected as arrays of spans grouped by trace id.
// But, since OTLP groups by arrays of spans from the same instrumentation library, not trace-id,
// (contrib-instrumention-redis, contrib-instrumention-rails, etc), we have to iterate
// over batch and group all spans by trace id.
// Second, otlp->dd conversion is what creates the resource name that we are checking in the blocklist.
// Note: OTLP also groups by ResourceSpans but practically speaking a payload will usually only
// contain 1 ResourceSpan array.

// Root span is used to carry some trace-level metadata, such as sampling rate and priority.
rootSpan := utils.GetRoot(apiTrace)

if !blk.Allows(rootSpan) {
//TODO: do we need to delete if we're skipping appending to payload?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should clarify this TODO before merging

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worked through it and I'm nearly sure its safe to not delete, updated the pr

delete(apiTraces, traceID)
continue
}

// calculates analyzed spans for use in trace search and app analytics
// appends a specific piece of metadata to these spans marking them as analyzed
// TODO: allow users to configure specific spans to be marked as an analyzed spans for app analytics
Expand Down Expand Up @@ -246,11 +268,13 @@ func spanToDatadogSpan(s pdata.Span,
// we can then set Error field when creating and predefine a max meta capacity
isSpanError := getSpanErrorAndSetTags(s, tags)

resourceName := getDatadogResourceName(s, tags)

span := &pb.Span{
TraceID: decodeAPMTraceID(s.TraceID().Bytes()),
SpanID: decodeAPMSpanID(s.SpanID().Bytes()),
Name: getDatadogSpanName(s, tags),
Resource: getDatadogResourceName(s, tags),
Resource: resourceName,
Service: normalizedServiceName,
Start: int64(startTime),
Duration: duration,
Expand Down
Loading