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.

Sign up for GitHub

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 units of time based metrics from millis to seconds for Java17 … #12084

Merged
merged 1 commit into from
Aug 28, 2024
Merged

Update units of time based metrics from millis to seconds for Java17 … #12084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .../main/java/io/opentelemetry/instrumentation/runtimemetrics/java17/internal/Constants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ private Constants() {}
public static final String ONE = "1";
public static final String HERTZ = "Hz";
public static final String BYTES = "By";
public static final String MILLISECONDS = "ms";
public static final String SECONDS = "s";
public static final String COMMITTED = "committed";
public static final String RESERVED = "reserved";
public static final String INITIAL_SIZE = "initialSize";
Expand Down
9 changes: 4 additions & 5 deletions ...in/java/io/opentelemetry/instrumentation/runtimemetrics/java17/internal/DurationUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,12 @@
* any time.
*/
public final class DurationUtil {
private static final double NANOS_PER_MILLI = 1e6;
private static final double NANOS_PER_SECOND = 1e9;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

elsewhere we use TimeUnit.SECONDS.toNanos(1)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is correct, I kept this scientific notation because it was already there. It may be a good idea to apply unified approach, however it would be even better to define it in a single place. I'll take a look at this. Thanks for the feedback.


/** Returns the duration as milliseconds, with fractional part included. */
@SuppressWarnings("TimeUnitMismatch")
public static double toMillis(Duration duration) {
/** Returns the duration as seconds, with fractional part included. */
public static double toSeconds(Duration duration) {
double epochSecs = (double) duration.getSeconds();
return epochSecs * 1000 + duration.getNano() / NANOS_PER_MILLI;
return epochSecs + duration.getNano() / NANOS_PER_SECOND;
}

private DurationUtil() {}
Expand Down
4 changes: 2 additions & 2 deletions .../io/opentelemetry/instrumentation/runtimemetrics/java17/internal/cpu/LongLockHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public LongLockHandler(Meter meter, ThreadGrouper grouper) {
meter
.histogramBuilder(METRIC_NAME)
.setDescription(METRIC_DESCRIPTION)
.setUnit(Constants.MILLISECONDS)
.setUnit(Constants.SECONDS)
.build();
}

Expand Down Expand Up @@ -73,7 +73,7 @@ public PerThreadLongLockHandler(DoubleHistogram histogram, String threadName) {
@Override
public void accept(RecordedEvent recordedEvent) {
if (recordedEvent.hasField(EVENT_THREAD)) {
histogram.record(DurationUtil.toMillis(recordedEvent.getDuration()), attributes);
histogram.record(DurationUtil.toSeconds(recordedEvent.getDuration()), attributes);
}
// What about the class name in MONITOR_CLASS ?
// We can get a stack trace from the thread on the event
Expand Down
9 changes: 4 additions & 5 deletions ...entation/runtimemetrics/java17/internal/garbagecollection/G1GarbageCollectionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
package io.opentelemetry.instrumentation.runtimemetrics.java17.internal.garbagecollection;

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.metrics.LongHistogram;
import io.opentelemetry.api.metrics.DoubleHistogram;
import io.opentelemetry.api.metrics.Meter;
import io.opentelemetry.instrumentation.runtimemetrics.java17.JfrFeature;
import io.opentelemetry.instrumentation.runtimemetrics.java17.internal.Constants;
Expand All @@ -27,21 +27,20 @@ public final class G1GarbageCollectionHandler implements RecordedEventHandler {
"G1 Young Generation",
Constants.ATTR_GC_ACTION,
Constants.END_OF_MINOR_GC);
private final LongHistogram histogram;
private final DoubleHistogram histogram;

public G1GarbageCollectionHandler(Meter meter) {
histogram =
meter
.histogramBuilder(Constants.METRIC_NAME_GC_DURATION)
.setDescription(Constants.METRIC_DESCRIPTION_GC_DURATION)
.setUnit(Constants.MILLISECONDS)
.ofLongs()
.setUnit(Constants.SECONDS)
.build();
}

@Override
public void accept(RecordedEvent ev) {
histogram.record(ev.getLong(Constants.DURATION), ATTR);
histogram.record(ev.getDouble(Constants.DURATION), ATTR);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robsunday I'm writing up the release notes for the upcoming release and had a question here, does this need a conversion from milliseconds to seconds? thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right - conversion should be added here and in all other modified handlers. Thanks for a great catch - I missed that. I'll update it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully resolved with #12244

}

@Override
Expand Down
9 changes: 4 additions & 5 deletions ...ntation/runtimemetrics/java17/internal/garbagecollection/OldGarbageCollectionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
package io.opentelemetry.instrumentation.runtimemetrics.java17.internal.garbagecollection;

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.metrics.LongHistogram;
import io.opentelemetry.api.metrics.DoubleHistogram;
import io.opentelemetry.api.metrics.Meter;
import io.opentelemetry.instrumentation.runtimemetrics.java17.JfrFeature;
import io.opentelemetry.instrumentation.runtimemetrics.java17.internal.Constants;
Expand All @@ -22,16 +22,15 @@
public final class OldGarbageCollectionHandler implements RecordedEventHandler {
private static final String EVENT_NAME = "jdk.OldGarbageCollection";

private final LongHistogram histogram;
private final DoubleHistogram histogram;
private final Attributes attributes;

public OldGarbageCollectionHandler(Meter meter, String gc) {
histogram =
meter
.histogramBuilder(Constants.METRIC_NAME_GC_DURATION)
.setDescription(Constants.METRIC_DESCRIPTION_GC_DURATION)
.setUnit(Constants.MILLISECONDS)
.ofLongs()
.setUnit(Constants.SECONDS)
.build();
// Set the attribute's GC based on which GC is being used.
attributes =
Expand All @@ -41,7 +40,7 @@ public OldGarbageCollectionHandler(Meter meter, String gc) {

@Override
public void accept(RecordedEvent ev) {
histogram.record(ev.getLong(Constants.DURATION), attributes);
histogram.record(ev.getDouble(Constants.DURATION), attributes);
}

@Override
Expand Down
9 changes: 4 additions & 5 deletions ...ation/runtimemetrics/java17/internal/garbagecollection/YoungGarbageCollectionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
package io.opentelemetry.instrumentation.runtimemetrics.java17.internal.garbagecollection;

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.metrics.LongHistogram;
import io.opentelemetry.api.metrics.DoubleHistogram;
import io.opentelemetry.api.metrics.Meter;
import io.opentelemetry.instrumentation.runtimemetrics.java17.JfrFeature;
import io.opentelemetry.instrumentation.runtimemetrics.java17.internal.Constants;
Expand All @@ -22,16 +22,15 @@
public final class YoungGarbageCollectionHandler implements RecordedEventHandler {
private static final String EVENT_NAME = "jdk.YoungGarbageCollection";

private final LongHistogram histogram;
private final DoubleHistogram histogram;
private final Attributes attributes;

public YoungGarbageCollectionHandler(Meter meter, String gc) {
histogram =
meter
.histogramBuilder(Constants.METRIC_NAME_GC_DURATION)
.setDescription(Constants.METRIC_DESCRIPTION_GC_DURATION)
.setUnit(Constants.MILLISECONDS)
.ofLongs()
.setUnit(Constants.SECONDS)
.build();
// Set the attribute's GC based on which GC is being used.
// G1 young collection is already handled by G1GarbageCollectionHandler.
Expand All @@ -42,7 +41,7 @@ public YoungGarbageCollectionHandler(Meter meter, String gc) {

@Override
public void accept(RecordedEvent ev) {
histogram.record(ev.getLong(Constants.DURATION), attributes);
histogram.record(ev.getDouble(Constants.DURATION), attributes);
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions ...ntelemetry/instrumentation/runtimemetrics/java17/internal/network/NetworkReadHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public NetworkReadHandler(Meter meter, ThreadGrouper nameNormalizer) {
meter
.histogramBuilder(Constants.METRIC_NAME_NETWORK_DURATION)
.setDescription(Constants.METRIC_DESCRIPTION_NETWORK_DURATION)
.setUnit(Constants.MILLISECONDS)
.setUnit(Constants.SECONDS)
.build();
}

Expand Down Expand Up @@ -81,7 +81,7 @@ public PerThreadNetworkReadHandler(
@Override
public void accept(RecordedEvent ev) {
bytesHistogram.record(ev.getLong(BYTES_READ), attributes);
durationHistogram.record(DurationUtil.toMillis(ev.getDuration()), attributes);
durationHistogram.record(DurationUtil.toSeconds(ev.getDuration()), attributes);
}
}
}
4 changes: 2 additions & 2 deletions ...telemetry/instrumentation/runtimemetrics/java17/internal/network/NetworkWriteHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public NetworkWriteHandler(Meter meter, ThreadGrouper nameNormalizer) {
meter
.histogramBuilder(Constants.METRIC_NAME_NETWORK_DURATION)
.setDescription(Constants.METRIC_DESCRIPTION_NETWORK_DURATION)
.setUnit(Constants.MILLISECONDS)
.setUnit(Constants.SECONDS)
.build();
}

Expand Down Expand Up @@ -99,7 +99,7 @@ private PerThreadNetworkWriteHandler(
@Override
public void accept(RecordedEvent ev) {
bytesHistogram.record(ev.getLong(BYTES_WRITTEN), attributes);
durationHistogram.record(DurationUtil.toMillis(ev.getDuration()), attributes);
durationHistogram.record(DurationUtil.toSeconds(ev.getDuration()), attributes);
}
}
}
4 changes: 2 additions & 2 deletions ...est/java/io/opentelemetry/instrumentation/runtimemetrics/java17/G1GcMemoryMetricTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import static io.opentelemetry.instrumentation.runtimemetrics.java17.internal.Constants.METRIC_NAME_GC_DURATION;
import static io.opentelemetry.instrumentation.runtimemetrics.java17.internal.Constants.METRIC_NAME_MEMORY;
import static io.opentelemetry.instrumentation.runtimemetrics.java17.internal.Constants.METRIC_NAME_MEMORY_AFTER;
import static io.opentelemetry.instrumentation.runtimemetrics.java17.internal.Constants.MILLISECONDS;
import static io.opentelemetry.instrumentation.runtimemetrics.java17.internal.Constants.SECONDS;
import static org.assertj.core.api.Assertions.assertThat;

import io.opentelemetry.api.common.Attributes;
Expand Down Expand Up @@ -88,7 +88,7 @@ void shouldHaveGcDurationMetrics() {
metric ->
metric
.hasName(METRIC_NAME_GC_DURATION)
.hasUnit(MILLISECONDS)
.hasUnit(SECONDS)
.hasDescription(METRIC_DESCRIPTION_GC_DURATION)
.satisfies(
data ->
Expand Down
4 changes: 2 additions & 2 deletions .../src/test/java/io/opentelemetry/instrumentation/runtimemetrics/java17/JfrCpuLockTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

package io.opentelemetry.instrumentation.runtimemetrics.java17;

import static io.opentelemetry.instrumentation.runtimemetrics.java17.internal.Constants.MILLISECONDS;
import static io.opentelemetry.instrumentation.runtimemetrics.java17.internal.Constants.SECONDS;
import static org.assertj.core.api.Assertions.assertThat;

import java.time.Duration;
Expand Down Expand Up @@ -51,7 +51,7 @@ void shouldHaveLockEvents() throws Exception {
metric ->
metric
.hasName("jvm.cpu.longlock")
.hasUnit(MILLISECONDS)
.hasUnit(SECONDS)
.hasHistogramSatisfying(histogram -> {}));
}
}
4 changes: 2 additions & 2 deletions ...est/java/io/opentelemetry/instrumentation/runtimemetrics/java17/PsGcMemoryMetricTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
import static io.opentelemetry.instrumentation.runtimemetrics.java17.internal.Constants.METRIC_NAME_MEMORY;
import static io.opentelemetry.instrumentation.runtimemetrics.java17.internal.Constants.METRIC_NAME_MEMORY_AFTER;
import static io.opentelemetry.instrumentation.runtimemetrics.java17.internal.Constants.METRIC_NAME_MEMORY_LIMIT;
import static io.opentelemetry.instrumentation.runtimemetrics.java17.internal.Constants.MILLISECONDS;
import static io.opentelemetry.instrumentation.runtimemetrics.java17.internal.Constants.SECONDS;
import static org.assertj.core.api.Assertions.assertThat;

import io.opentelemetry.api.common.Attributes;
Expand Down Expand Up @@ -93,7 +93,7 @@ void shouldHaveGcDurationMetrics() {
metric ->
metric
.hasName(METRIC_NAME_GC_DURATION)
.hasUnit(MILLISECONDS)
.hasUnit(SECONDS)
.hasDescription(METRIC_DESCRIPTION_GC_DURATION)
.satisfies(
data ->
Expand Down
4 changes: 2 additions & 2 deletions ...java/io/opentelemetry/instrumentation/runtimemetrics/java17/SerialGcMemoryMetricTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import static io.opentelemetry.instrumentation.runtimemetrics.java17.internal.Constants.END_OF_MINOR_GC;
import static io.opentelemetry.instrumentation.runtimemetrics.java17.internal.Constants.METRIC_DESCRIPTION_GC_DURATION;
import static io.opentelemetry.instrumentation.runtimemetrics.java17.internal.Constants.METRIC_NAME_GC_DURATION;
import static io.opentelemetry.instrumentation.runtimemetrics.java17.internal.Constants.MILLISECONDS;
import static io.opentelemetry.instrumentation.runtimemetrics.java17.internal.Constants.SECONDS;
import static org.assertj.core.api.Assertions.assertThat;

import io.opentelemetry.api.common.Attributes;
Expand Down Expand Up @@ -43,7 +43,7 @@ void shouldHaveGcDurationMetrics() {
metric ->
metric
.hasName(METRIC_NAME_GC_DURATION)
.hasUnit(MILLISECONDS)
.hasUnit(SECONDS)
.hasDescription(METRIC_DESCRIPTION_GC_DURATION)
.satisfies(
data ->
Expand Down
26 changes: 26 additions & 0 deletions ...ava/io/opentelemetry/instrumentation/runtimemetrics/java17/internal/DurationUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.runtimemetrics.java17.internal;

import static org.junit.jupiter.api.Assertions.assertEquals;

import java.time.Duration;
import org.junit.jupiter.api.Test;

class DurationUtilTest {

@Test
void shouldConvertDurationToSeconds() {
// Given
Duration duration = Duration.ofSeconds(7, 144);

// When
double seconds = DurationUtil.toSeconds(duration);

// Then
assertEquals(7.000000144, seconds);
}
}
Loading