Skip to content
This repository has been archived by the owner on Aug 30, 2019. It is now read-only.

filters: apply replace rules only after obfuscation #477

Merged
merged 2 commits into from
Sep 28, 2018
Merged

Conversation

gbbr
Copy link
Contributor

@gbbr gbbr commented Sep 26, 2018

Fixes #462

Problem:

  • When using replace rules to modify the resource of an SQL span, the resulting sql.query tag created by the agent is also affected by the replace rules because it is a copy of the resulting resource. This change moves the replacer after obfuscation, when the sql.query tag is already created, leaving the original (obfuscated) query visible to the user. This also makes more sense from a UX point of view because our users will want to provide regular expressions for replacements based on what they see in the UI, which is the post-obfuscation entry.

Changes to filter package:

  • Removes Filter interface
  • Exports resourceFilter as Blacklister
  • Exports tagReplacer as Replacer

Changes to processor:

  • Instead of running a list of filters (which weren't all filters to begin with), keep an instance of the Replacer and Blacklister and run them in the correct order.
  • Run Blacklister early and Replacer only after obfuscation.

Copy link

@tylerbenson tylerbenson left a comment

Choose a reason for hiding this comment

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

Few questions and comments, but from what I can see as a go newb it seems ok.

@@ -50,7 +48,8 @@ func (pt *processedTrace) getSamplingPriority() (int, bool) {
type Agent struct {
Receiver *HTTPReceiver
Concentrator *Concentrator
Filters []filters.Filter
Blacklister *filters.Blacklister

Choose a reason for hiding this comment

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

Can I suggest Denylister instead?
rails/rails#33677

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'm more along the lines of the second comment in that thread.

Choose a reason for hiding this comment

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

Still worth considering though.

cfg.ReplaceTags = []*config.ReplaceRule{{
Name: "resource.name",
Re: regexp.MustCompile("AND.*"),
Repl: "...",

Choose a reason for hiding this comment

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

This replacement is something that would normally break the parser for normalization, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly, although that wasn't the case being tested here. But you're right, this is yet another bug that is resolved by this change.

@gbbr gbbr merged commit cd22ce3 into master Sep 28, 2018
@gbbr gbbr deleted the gbbr/replace-after branch September 28, 2018 14:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants