[exporter/datadog]: add ignore_resources configuration option #3245
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3245 +/- ##
==========================================
- Coverage 91.91% 91.84% -0.07%
==========================================
Files 494 494
Lines 23939 23980 +41
==========================================
+ Hits 22003 22024 +21
- Misses 1429 1449 +20
Partials 507 507
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
cc @mx-psi for review from datadog |
| rootSpan := utils.GetRoot(apiTrace) | ||
|
|
||
| if !blk.Allows(rootSpan) { | ||
| //TODO: do we need to delete if we're skipping appending to payload? |
There was a problem hiding this comment.
We should clarify this TODO before merging
There was a problem hiding this comment.
I worked through it and I'm nearly sure its safe to not delete, updated the pr
| if c.Traces.IgnoreResources == nil { | ||
| c.Traces.IgnoreResources = []string{} | ||
| } | ||
|
|
There was a problem hiding this comment.
A nil slice works pretty much like an empty slice, I think we can remove this
| if err != nil { | ||
| // log.Errorf("Invalid resource filter: %q", entry) | ||
| continue | ||
| } |
There was a problem hiding this comment.
We should handle this instead of just ignoring it. I think we can
- On the configuration, validate the regexes using
regexp.Compile. Following addvalidatableinterface to all the component config opentelemetry-collector#2898 we should do this on aValidatemethod from the configuration. - Use
regexp.MustCompileand ensure that we only pass valid regexes to theNewBlocklister.
There was a problem hiding this comment.
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
…g conventions, clean up code comments
| rule, err := regexp.Compile(entry) | ||
| if err != nil { | ||
| // log.Errorf("Invalid resource filter: %q", entry) | ||
| continue | ||
| } |
There was a problem hiding this comment.
What I meant with the comment above is that we can now just do
| rule, err := regexp.Compile(entry) | |
| if err != nil { | |
| // log.Errorf("Invalid resource filter: %q", entry) | |
| continue | |
| } | |
| rule := regexp.MustCompile(entry) |
since we know the regexes are valid
There was a problem hiding this comment.
kk makes sense, updated this and cleaned up the tests. i did have to remove a few tests from denylist_test.go that tested invalid regex, but moved them into the config_test section for the Validate function, which I think is where they'd belong now anyway.
|
👋 @kbrockhoff could i get a quick review here when u have a moment? |
|
LGTM |
…elemetry#3245) * [exporter/datadog]: wip add blocklist for resources. wip test and documentation * [exporter/datadog]: clean up ignore resources defaults * [exporter/datadog]: remove unused helpers * [exporter/datadog]: add yaml config details * [exporter/datadog]: add links to datadog agent funcs * [exporter/datadog]: seperate blocklist into its own file and tests * [exporter/datadog]: fix copyrights * [exporter/datadog]: pr feedback update code comments and update naming conventions, clean up code comments * [exporter/datadog]: add validatioon block to config * [exporter/datadog]: update config * [exporter/datadog]: linting * [exporter/datadog]: wip add helper tests * [exporter/datadog]: add helper tests * [exporter/datadog]: linting * [exporter/datadog]: use mustcompile for denylist * [exporter/datadog]: fix tests for new config/denylist behavior * [exporter/datadog]: add ignore resources validation test for config
Description:
This PR adds a configuration option
ignore_resources, which matches the feature currently available in the datadog-agent, to the opentelemetry-collector datadogexporter. The configuration option allows users to supply A blocklist of regular expressions to "drop" certain traces based on their resource name. Aresource nameis a Datadog APM Specific concept which generally represents a specific endpoint or route in an application, such aGET /healthcheck.This feature has been requested by a number of end users and allows users greater flexibility for "dropping" certain low value traces before they generate trace related metrics/stats or are added to trace payloads sent to datadog. It allows users to control any associated ingestion costs from sending data to the datadog backend, as well as filter out unwanted traces.
In the context of the OpenTelemetry Collector, specifically how
ignore_resourcesfunctions is it will drop the entire "trace chunk" (all spans with the same traceId in a payload of ResourceSpans), when the "Root" span in that trace has a resource that is in the blocklist.Technically, this means that in an environment with very low or zero batching within the tracing pipeline (at sdk processor/exporter or in collector pipeline processors), some child spans of the blocklisted trace may not be dropped (since they'd be contained in a separate payload). Generally speaking there's not much we can do about this limitation and I think, since the datadogexporter's suggested setup/configuration instructions include specific requirements and caveats to ensure batching, this is an acceptable limitation/tradeoff
*Note: For context, The implementation is similar to the datadog-agent
blacklister, with some differences I tried to note through code comments and above. I wanted to try to use more inclusive language (blacklist-> blocklist). It does admittedly sound a little odd so if there's more appropriate terminology here i'm open to changing.Testing: Added a unit test and tests for the blocklister functionality
Documentation: Added configuration option to the example config yaml. It may be worth adding a section to the
READMEwith usage instructions as well