-
Notifications
You must be signed in to change notification settings - Fork 130
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
Fix units for some metrics #140
Conversation
public static final String KILOBYTES = "kb"; | ||
public static final String HERTZ = "Hz"; | ||
public static final String BYTES = "B"; | ||
public static final String KILOBYTES = "kB"; |
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 have a related question for this (but not necessarily strictly the subject of this PR).
Is kB
and MB
are actually KiB
and MiB
?
See kibibyte vs. kilobyte: https://en.wikipedia.org/wiki/Byte#Multiple-byte_units
1kB == 1000B
1KiB == 1024B
If they are KiB
/MiB
, I would either change them or add some javadoc here that kB and MB are based on powers of 2 not 10 because based on the international standards 1kB is based on powers of 10).
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.
They are actually KiB and MiB. This event:
jdk.GCHeapSummary {
startTime = 11:51:47.587
gcId = 685
when = "After GC"
heapSpace = {
start = 0x7C0000000
committedEnd = 0x7D9300000
committedSize = 403.0 MB
reservedEnd = 0x800000000
reservedSize = 1.0 GB
}
heapUsed = 155.7 MB
}
is actually
Used: 163273232
when read as a raw long (which actually returns bytes), and so:
jshell> 155.7 * 1024 * 1024
$2 ==> 1.632632832E8
I'll fix this - looks like we should use bytes everywhere and format for display.
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.
looks like we should use bytes everywhere and format for display
👍
Description:
I noticed that some of our units in the jfr-streaming module are not quite correct. I have made them be correct, but I don't know if all of the units I've defined (e.g. Hz) are actually used in OpenTelemetry.
Testing:
Manual test of each JFR event that the code responds to.