Skip to content

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

Merged
trask merged 9 commits into
open-telemetry:masterfrom
johnbley:master
May 4, 2020
Merged

Normalize sql statements to elide literal numbers and strings.#366
trask merged 9 commits into
open-telemetry:masterfrom
johnbley:master

Conversation

@johnbley

Copy link
Copy Markdown
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.

Comment thread agent-bootstrap/src/main/java/io/opentelemetry/auto/config/Config.java Outdated

@trask trask left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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

johnbley commented Apr 29, 2020

Copy link
Copy Markdown
Member Author

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

trask commented Apr 30, 2020

Copy link
Copy Markdown
Member

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

trask commented Apr 30, 2020

Copy link
Copy Markdown
Member

check finally succeeded! 😆🎉

@johnbley

Copy link
Copy Markdown
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.
schmikei pushed a commit to schmikei/opentelemetry-java-instrumentation that referenced this pull request Apr 17, 2025
Co-authored-by: Joao Grassi <joao.grassi@dynatrace.com>
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