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

Normalize sql statements to elide literal numbers and strings. #366

Merged
merged 9 commits into from
May 4, 2020
Merged

Normalize sql statements to elide literal numbers and strings. #366

merged 9 commits into from
May 4, 2020

Conversation

johnbley
Copy link
Member

Normalize sql statements using a javacc-based lexer.

  • Replace sql numeric and string literals with a ?
  • Unit tests for sql normalization, including edge cases like thousands of quote marks
  • Adjust existing instrumentation tests accordingly to expect normalized sql in spans
  • Add configuration option to disable this feature
  • Fix weird edge case in muzzle: ReferenceMatcher's handling of primitive values was preventing helper classes from using primitive fields (primitive arrays were fine) due to two different reflective type systems in use with different representations of said primitive types.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

This all looks really good to me 👍

I think it would be nice for the normalization to happen off the main thread (in a span processor pipeline, prior to sending to exporter), but we can revisit that later (need to address #368 first 😄).

@trask trask added this to the v0.3.0 milestone Apr 29, 2020
@johnbley
Copy link
Member Author

I think it would be nice for the normalization to happen off the main thread (in a span processor pipeline, prior to sending to exporter), but we can revisit that later (need to address #368 first 😄).

It's pretty darn fast (sub-sub-millisecond for normal sql), and could be made faster if need be. But I agree, we're going to encounter things like this that necessitate some off-thread processing.

@johnbley
Copy link
Member Author

johnbley commented Apr 29, 2020

Can any gradle/circleci expert on here help with the spotless license check here? The issue is that javacc generates some files in build/generated/, and the check target is complaining that they don't have a license header. I specifically saw this problem on my local builds and added an exclude for **/generated/** to the top-level gradle/spotless.gradle config. This works for my builds but apparently not for circleci ones?

@trask
Copy link
Member

trask commented Apr 30, 2020

I can confirm. Works locally. Fails on CI. Really not sure what's the difference. Looked a bit at the spotless gradle plugin source code even 😭.

Some ideas worth trying (in this order of invasiveness I think):

  • Update com.diffplug.gradle.spotless to the latest 3.28.1 in trace-java.gradle
  • Change targetExclude from **/generated/** to build/generated/**
  • Change targetExclude again, this time to build/generated/**/*.java
  • Remove targetExclude altogether and add target 'src/**/*.java'

I think the last one should work, it's not ideal as it bypasses spotless' built-in detection of Java source sets, but if the other ideas don't work, then I'm all in on the last one 👍.

@trask
Copy link
Member

trask commented Apr 30, 2020

check finally succeeded! 😆🎉

@johnbley
Copy link
Member Author

check finally succeeded! 😆🎉

Yes. I don't know if you saw the commit comment, but I did manually make sure that a missing license header in src/* does in fact still fail the build.

@trask trask merged commit ca296b9 into open-telemetry:master May 4, 2020
iNikem pushed a commit to iNikem/opentelemetry-java-instrumentation that referenced this pull request May 5, 2020
…telemetry#366)

* Normalize sql statements to elide literal numbers and strings.

* Missed one SlickTest sql normalization.

* Fix muzzle order for helper classes.

* Change name of feature flag

* Upgrade to latest spotless version in an attempt #1 to make the circleci build work.

* Attempt 2 to make circleci build happy - exclude build/generated/** from spotless.

* Attempt 3 to get circleci build working, adding *.java to the exclude line.

* Change exclude of generated files to include of just the src/ directory.
I confirmed that this properly failed the build if I remove a license header from a src/ directory.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants