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

obfuscate: clean up SQL obfuscator #487

Merged
merged 1 commit into from
Oct 10, 2018
Merged

obfuscate: clean up SQL obfuscator #487

merged 1 commit into from
Oct 10, 2018

Conversation

gbbr
Copy link
Contributor

@gbbr gbbr commented Oct 10, 2018 •
edited
Loading

  • Renaming and simplifying comments
  • Removed agent.parse.error tag as it brought no real details about parsing errors.
  • Reset obfuscator only once, before it runs.

No apparent impact on performance:

name                      old time/op    new time/op    delta
Tokenizer/Escaping/512-8    7.27µs ± 0%    7.30µs ± 0%  +0.32%
Tokenizer/Grouping/199-8    4.16µs ± 0%    4.17µs ± 0%  +0.19%

name                      old alloc/op   new alloc/op   delta
Tokenizer/Escaping/512-8    4.40kB ± 0%    4.40kB ± 0%   0.00%
Tokenizer/Grouping/199-8    2.72kB ± 0%    2.72kB ± 0%   0.00%

name                      old allocs/op  new allocs/op  delta
Tokenizer/Escaping/512-8      87.0 ± 0%      87.0 ± 0%   0.00%
Tokenizer/Grouping/199-8      68.0 ± 0%      68.0 ± 0%   0.00%

@gbbr gbbr requested a review from palazzem October 10, 2018 08:56
palazzem
palazzem reviewed Oct 10, 2018
View reviewed changes
Copy link

@palazzem palazzem left a comment

Choose a reason for hiding this comment

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

Great cleanup!

obfuscate/sql.go

func (t *sqlObfuscator) obfuscate(in string) (string, error) {
var out bytes.Buffer
t.reset(in)
Copy link

Choose a reason for hiding this comment

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

Is it enough to reset it once? I guess it was redundant, but can you check the benchmarks (I think we should have something) to detect how many allocations / performance we have? Just for posterity, since this is a critical component that is called so many times.

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 have updated the PR description. There is a small difference in time, but I think that's negligible and most likely unrelated to the change. Happy to address performance improvements in another PR. How's it look?

@gbbr
Copy link
Contributor Author

gbbr commented Oct 10, 2018

@palazzem PTAL

@gbbr gbbr mentioned this pull request Oct 10, 2018
Closed
@gbbr gbbr merged commit bda1ebb into master Oct 10, 2018
@gbbr gbbr deleted the gbbr/cleanup-sql branch October 10, 2018 13:53
@gbbr gbbr added this to the 6.6.0 milestone Oct 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@palazzem palazzem palazzem approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
6.6.0
Development

Successfully merging this pull request may close these issues.
2 participants
@gbbr @palazzem