-
Notifications
You must be signed in to change notification settings - Fork 839
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
Cache sql sanitized extraction #2094
Conversation
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. |
.../jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/JdbcUtils.java
Outdated
Show resolved
Hide resolved
Muzzle failed locally:
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? |
.../jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/JdbcUtils.java
Outdated
Show resolved
Hide resolved
.../jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/JdbcUtils.java
Outdated
Show resolved
Hide resolved
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 |
3ab7896
to
8dfda98
Compare
Closing to prevent premature merging (until I can get a muzzle fix in). |
Going to try this as a draft instead of closing. |
javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/GuavaBoundedCache.java
Outdated
Show resolved
Hide resolved
.../jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/JdbcUtils.java
Outdated
Show resolved
Hide resolved
.../jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/JdbcUtils.java
Outdated
Show resolved
Hide resolved
javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentTooling.java
Outdated
Show resolved
Hide resolved
javaagent-tooling/src/test/java/io/opentelemetry/javaagent/tooling/GuavaBoundedCacheTest.java
Outdated
Show resolved
Hide resolved
8d33aaf
to
863097a
Compare
…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.
…ling/AgentTooling.java Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>
…javaagent/instrumentation/jdbc/JdbcUtils.java Co-authored-by: Anuraag Agrawal <anuraaga@gmail.com>
cb7b918
to
2bd4596
Compare
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:
(all times in ms)