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

Cache sql sanitized extraction #2094

Merged
merged 12 commits into from
Jan 27, 2021

Conversation

breedx-splk
Copy link
Contributor

This builds on top of #2087 and will be rebased when that is in...so maybe wait to review?

The discussion in #2065 talks about sql parsing being expensive. If we cache the sql statement string to SqlStatementInfo we can prevent having to do this heavy lifting more than once for each statement. A guava LRU cache was selected to prevent caching excessive numbers of statements on the heap in the even the user does dynamic/inconsistent sql string creation.

This seems like a pretty reasonable compromise -- it keeps the utility of sanitization and db/operation extraction while preventing duplicate efforts in common situations.

A benchmark based on spring-petclinic-rest (which uses hibernate) shows the following with 5VUs, 500 iterations:

  • No agent : avg = 98.83 p95 = 119.73
  • With agent no cache: avg = 319.32 p95 = 525.50
  • With agent with cache: avg = 129.67 p95 = 174.81
    (all times in ms)

@jkwatson
Copy link
Contributor

This seems like a pretty nice performance win. 5000 might even be too many to cache. If we're willing to put metrics into our code, it would be rad to register an observer that would report how many items were in the cache.

@mateuszrzeszutek
Copy link
Member

Muzzle failed locally:

FAILED MUZZLE VALIDATION: io.opentelemetry.javaagent.instrumentation.jdbc.JdbcInstrumentationModule mismatches:
-- io.opentelemetry.javaagent.instrumentation.jdbc.JdbcUtils:31 Missing class com.google.common.cache.CacheBuilder
-- io.opentelemetry.javaagent.instrumentation.jdbc.JdbcUtils:79 Missing class com.google.common.cache.Cache

This is the second time I've noticed that it passes in GHA but fails (correctly) when run locally. Do we use some sort of cache for muzzle tasks?

@iNikem
Copy link
Contributor

iNikem commented Jan 22, 2021

Muzzle failed locally:

FAILED MUZZLE VALIDATION: io.opentelemetry.javaagent.instrumentation.jdbc.JdbcInstrumentationModule mismatches:
-- io.opentelemetry.javaagent.instrumentation.jdbc.JdbcUtils:31 Missing class com.google.common.cache.CacheBuilder
-- io.opentelemetry.javaagent.instrumentation.jdbc.JdbcUtils:79 Missing class com.google.common.cache.Cache

This is the second time I've noticed that it passes in GHA but fails (correctly) when run locally. Do we use some sort of cache for muzzle tasks?

All GHA runs use gradle cache. Both remote and "local". Seems like muzzle tasks don't play nicely with caches?

@mateuszrzeszutek
Copy link
Member

mateuszrzeszutek commented Jan 25, 2021

All GHA runs use gradle cache. Both remote and "local". Seems like muzzle tasks don't play nicely with caches?

I've created an issue for that: #2110

@breedx-splk
Copy link
Contributor Author

Closing to prevent premature merging (until I can get a muzzle fix in).

@breedx-splk
Copy link
Contributor Author

Going to try this as a draft instead of closing.

@breedx-splk breedx-splk reopened this Jan 25, 2021
@breedx-splk breedx-splk marked this pull request as draft January 25, 2021 18:05
@breedx-splk breedx-splk marked this pull request as ready for review January 25, 2021 23:31
Base automatically changed from master to main January 26, 2021 05:50
…itization/extraction.

# Conflicts:
#	instrumentation/jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/JdbcUtils.java
…nstrumenation on guava (which apparently breaks muzzle). mostly merging to see if CI has the same problem with muzzle.
breedx-splk and others added 7 commits January 26, 2021 14:58
…ling/AgentTooling.java

Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>
…javaagent/instrumentation/jdbc/JdbcUtils.java

Co-authored-by: Anuraag Agrawal <anuraaga@gmail.com>
@trask trask merged commit 782a646 into open-telemetry:main Jan 27, 2021
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.

None yet

6 participants