-
Notifications
You must be signed in to change notification settings - Fork 80
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
Update object allocation stats so it works, but disabled by default #2146
Conversation
263e23e
to
4c2ea6c
Compare
Also adds guava as an explicit dependency to other modules, which previously implicitly picked up guava from the old java-allocation-tracker library version. Fixes deephaven#2120
4c2ea6c
to
cab8d68
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May approve as-is, depending on follow-up comments.
@@ -20,6 +20,8 @@ dependencies { | |||
|
|||
compileOnly 'javax.inject:javax.inject:1' | |||
|
|||
implementation 'com.google.guava:guava:19.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this was depended on implicitly before, but I'm wondering if we want to try and remove our dependency on guava, or update to a more recent version? (19.0 from 2015)
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-8908
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-10237
At the very least, we should have a follow-up ticket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this was picked to ensure we don't change our current version.
We are not susceptible to either cve, I actually checked this while waiting for a build - we don't use the classes required by either, and for CVE-2018-10237 we don't allow guava objects to be serialized and sent to us at all at this time (w2w might support this in theory some day, but java serialization is already a minefield, and we would want to only enable that feature for trusted other servers).
* | ||
* To use make sure you set | ||
* | ||
* -j -Dallocation.stats.enabled=true -j -Xbootclasspath/a:java_lib/allocation-2.0.jar -j | ||
* -javaagent:java_lib/allocation-2.0.jar (maybe even use -j -noverify) | ||
* -Dallocation.stats.enabled=true -javaagent:/opt/deephaven/server/lib/java-allocation-instrumenter-3.3.0.jar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment implies we are adding the jar to our classpath - but since it's compileOnly, we aren't; yeah?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making this compileOnly makes the feature opt-in, and we want it off by default - without the javaagent set, it doesnt matter if you turn enable allocation.stats.enabled
or not, nothing will be recorded).
The act of passing javaagent both a) adds that (shaded) jar to the classpath and b) enables the jar's premain to run and instrument the real classpath. As a side effect of b), we will see the jar from our own classpath.
The path in that documentation string is just a hypothetical, but we don't ship the jar (or its ... incorrect dependency tree) at all - I have a comment on #2120 that describes how to actually enable this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Also adds guava as an explicit dependency to other modules, which
previously implicitly picked up guava from the old
java-allocation-tracker library version.
Fixes #2120