filters: add a span tag replace filters #398
Conversation
gbbr
force-pushed
the
gbbr/tag-filters
branch
2 times, most recently
from
fea8f62 to
1a3aaf8
Compare
|
@LotharSee I've changed this. Would be great if you could take a look. |
config/merge_yaml.go
Outdated
| return md, nil | ||
| } | ||
|
|
||
| func ParseReplaceRulesFromString(rules [][3]string) (map[string]map[*regexp.Regexp]string, error) { |
There was a problem hiding this comment.
We should move this function from here. Either to the test package where it is used, or by converting the tests to the new format.
| "github.com/DataDog/datadog-trace-agent/model" | ||
| ) | ||
|
|
||
| var _ Filter = (*tagReplacer)(nil) |
There was a problem hiding this comment.
It is a common pattern that shows that the structure implements the Filter interface, as well as tests that by failing at compile time.
For the latter it may many times not be needed because other code might be already using this structure as that interface, in which case compiling will fail anyway. But I have made it a habit to do this because in many cases that doesn't happen. I don't feel strongly about it but I think it's a good thing to have.
It is used a lot in the standard library too. Check this example.
config/agent.go
Outdated
|
|
||
| // RawReplaceTags holds the unparsed replace rules. They are here for expvar. | ||
| RawReplaceTags []*replaceRule `json:"ReplaceTags"` | ||
|
|
There was a problem hiding this comment.
What about using a structure to wrap these two?
Something similar to replaceRule could also hold the its built Regexp then a list of it would be enough to be stored in the config. It allows both to have have a single struct and place per rule, and to have something simpler than map[string]map[*regexp.Regexp]string.
As tagReplacer.Keep is iterating over the tags anyway, it doesn't change much to keep only a list.
That's a nitpick though.
There was a problem hiding this comment.
Yeah I wasn't too pleased about this either, I think I've iterated a bit too much over the code and it became messy. I also "copied" the PR from our internal ticket a little bit. I like your suggestion. I'm gonna try it out.
There was a problem hiding this comment.
Actually what you've suggested in here worked, and I've added. PTAL. And thanks, it helps to have a second pair of eyes. I wasn't very happy with the initial version but I think now it's looking better.
config/merge_yaml.go
Outdated
| @@ -288,6 +306,25 @@ func readExponentialBackoffConfigYaml(yc queueablePayloadSender) backoff.Exponen | |||
| return c | |||
| } | |||
|
|
|||
| // parseReplaceRules takes a set of replace rules, validates and parses them, | |||
| // returning the corresponding map containing compiled regular expressions. | |||
| func parseReplaceRules(rules []*ReplaceRule) error { | |||
There was a problem hiding this comment.
I will rename this to "compileReplaceRules" and change the docs too after your review. I don't want to do that while you are potentially looking :)
|
FYI the following config: Tested on staging: |
This change adds a new setting (only Agent 6 for now) which allows to
set regular expressions in order to parse tag values. This helps hide
sensitive information from the UI. A new configuration example is:
```
apm_config:
# ...
# allows the same tag multiple times
# the replacement should be a user defined string
# this list of regex applies to all Spans of a trace
replace_tags:
- name: "*"
pattern: "user_id/([^/]*)"
repl: "?"
- name: "http.url"
pattern: "token/(.*)"
repl: "?"
- name: "http.url"
pattern: "guid"
repl: "[REDACTED]"
- name: "custom.tag"
pattern: "something"
repl: "something_else"
```
|
Thanks for your help! |
This change adds a new setting (only Agent 6 for now) which allows to
set regular expressions in order to parse tag values. This helps hide
sensitive information from the UI. A new configuration example is: