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

Fix units for some metrics #140

Merged
merged 3 commits into from
Dec 1, 2021
Merged

Conversation

kittylyst
Copy link
Contributor

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.

@kittylyst kittylyst requested a review from a team November 24, 2021 16:47
public static final String KILOBYTES = "kb";
public static final String HERTZ = "Hz";
public static final String BYTES = "B";
public static final String KILOBYTES = "kB";
Copy link
Member

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).

Copy link
Contributor Author

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.

Copy link
Member

@trask trask left a 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

👍

@trask trask merged commit 08ecf83 into open-telemetry:main Dec 1, 2021
@trask trask mentioned this pull request Dec 2, 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.

3 participants