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

feat: introduce java.time methods #1729

Merged
merged 7 commits into from
Nov 26, 2024
+44 −18
Merged

feat: introduce java.time methods #1729

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e61c325
feat: introduce `java.time` methods
diegomarquezp Nov 19, 2024
b521713
feat: introduce `java.time` methods
diegomarquezp Nov 19, 2024
37e767b
Merge branch 'introduce-java-time' of https://github.com/googleapis/j…
diegomarquezp Nov 19, 2024
d1feef2
use add-opens to allow serialization of java.time.Duration
diegomarquezp Nov 22, 2024
5a5abd4
use add-opens in surefire only when running in jdk9+
diegomarquezp Nov 22, 2024
2b09a03
Revert "use add-opens in surefire only when running in jdk9+"
diegomarquezp Nov 22, 2024
46b9172
Revert "use add-opens to allow serialization of java.time.Duration"
diegomarquezp Nov 22, 2024
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
28 changes: 22 additions & 6 deletions google-cloud-logging/src/main/java/com/google/cloud/logging/HttpRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,18 @@

package com.google.cloud.logging;

import static com.google.api.gax.util.TimeConversionUtils.toJavaTimeDuration;
import static com.google.api.gax.util.TimeConversionUtils.toThreetenDuration;

import com.google.api.core.ApiFunction;
import com.google.api.core.ObsoleteApi;
import com.google.cloud.StringEnumType;
import com.google.cloud.StringEnumValue;
import com.google.common.base.MoreObjects;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import java.io.Serializable;
import java.time.Duration;
import java.util.Objects;
import org.threeten.bp.Duration;

/**
* Objects of this class represent information about the (optional) HTTP request associated with a
Expand Down Expand Up @@ -51,7 +55,7 @@ public final class HttpRequest implements Serializable {
private final boolean cacheHit;
private final boolean cacheValidatedWithOriginServer;
private final Long cacheFillBytes;
private final Duration latency;
private final java.time.Duration latency;

/** The HTTP request method. */
public static final class RequestMethod extends StringEnumValue {
Expand Down Expand Up @@ -112,7 +116,7 @@ public static final class Builder {
private boolean cacheHit;
private boolean cacheValidatedWithOriginServer;
private Long cacheFillBytes;
private Duration latency;
private java.time.Duration latency;

Builder() {}

Expand Down Expand Up @@ -258,12 +262,18 @@ public Builder setCacheFillBytes(long cacheFillBytes) {
return this;
}

/** This method is obsolete. Use {@link #setLatencyDuration(java.time.Duration)} instead. */
@ObsoleteApi("Use setLatencyDuration(java.time.Duration) instead")
public Builder setLatency(org.threeten.bp.Duration latency) {
return setLatencyDuration(toJavaTimeDuration(latency));
}

/**
* Sets the latency on the server, from the time the request was received until the response was
* sent.
*/
@CanIgnoreReturnValue
public Builder setLatency(Duration latency) {
public Builder setLatencyDuration(java.time.Duration latency) {
this.latency = latency;
return this;
}
Expand Down Expand Up @@ -393,13 +403,19 @@ public Long getCacheFillBytes() {
return cacheFillBytes;
}

/** This method is obsolete. Use {@link #getLatencyDuration()} instead. */
@ObsoleteApi("Use getLatencyDuration() instead")
public org.threeten.bp.Duration getLatency() {
return toThreetenDuration(getLatencyDuration());
}

/**
* Returns the processing latency on the server, from the time the request was received until the
* response was sent.
*
* @return the latency, for null if not populated.
*/
public Duration getLatency() {
public Duration getLatencyDuration() {
return latency;
}

Expand Down Expand Up @@ -561,7 +577,7 @@ static HttpRequest fromPb(com.google.logging.type.HttpRequest requestPb) {
}
if (requestPb.hasLatency()) {
// NOTE(pongad): Don't convert to nano; large durations overflow longs!
builder.setLatency(
builder.setLatencyDuration(
Duration.ofSeconds(
requestPb.getLatency().getSeconds(), requestPb.getLatency().getNanos()));
}
Expand Down
10 changes: 10 additions & 0 deletions google-cloud-logging/src/main/java/com/google/cloud/logging/LogEntry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import com.google.logging.v2.LogEntrySourceLocation;
import com.google.logging.v2.LogName;
import java.io.Serializable;
import java.time.Duration;
import java.time.Instant;
import java.util.HashMap;
import java.util.Map;
Expand Down Expand Up @@ -612,6 +613,14 @@ public JsonElement serialize(
}
}

static final class DurationSerializer implements JsonSerializer<Duration> {
@Override
public JsonElement serialize(
Duration src, java.lang.reflect.Type typeOfSrc, JsonSerializationContext context) {
return new JsonPrimitive(src.toString());
}
}
Comment on lines +616 to +622
Copy link
Contributor

Choose a reason for hiding this comment

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

qq, why is this DurationSerializer added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default, the Gson serializer will try to access private methods/fields of Duration (and Instant - see the already-existing seralizer above). This seralizer relies on toString() instead.

Copy link
Contributor

@lqiu96 lqiu96 Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

By default, the Gson serializer will try to access private methods/fields of Duration

I'm not following this part. Why does it try to access these methods + fields from Duration? Also, why is this an issue now and not previously?

Is this related to an error message/ stacktrace from a test?

Copy link
Contributor Author

@diegomarquezp diegomarquezp Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why does it try to access these methods + fields from Duration?

I think it's meant to create a JSON string by accessing all its fields (i.e. {"seconds":123,"nanos":123}

Also, why is this an issue now and not previously?

I'm trying to figure this out. The JDK version has both fields as private. So does the backport. Both also implement the same interfaces (Serializable, Comparable, TemporalAmount.
The failing call happens in GSON when it tries to make the fields accessible - see below.

Is this related to an error message/ stacktrace from a test?

com.google.gson.JsonIOException: Failed making field 'java.time.Duration#seconds' accessible; either increase its visibility or write a custom TypeAdapter for its declaring type.
See https://github.com/google/gson/blob/main/Troubleshooting.md#reflection-inaccessible

	at com.google.gson.internal.reflect.ReflectionHelper.makeAccessible(ReflectionHelper.java:76)
	at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory.getBoundFields(ReflectiveTypeAdapterFactory.java:388)
	at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory.create(ReflectiveTypeAdapterFactory.java:161)
	at com.google.gson.Gson.getAdapter(Gson.java:628)
	at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory.createBoundField(ReflectiveTypeAdapterFactory.java:201)
	at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory.getBoundFields(ReflectiveTypeAdapterFactory.java:395)
	at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory.create(ReflectiveTypeAdapterFactory.java:161)
	at com.google.gson.Gson.getAdapter(Gson.java:628)
	at com.google.gson.Gson.toJson(Gson.java:928)
	at com.google.gson.Gson.toJson(Gson.java:899)
	at com.google.gson.Gson.toJson(Gson.java:848)
	at com.google.gson.Gson.toJson(Gson.java:825)
	at com.google.cloud.logging.LogEntry$StructuredLogFormatter.appendField(LogEntry.java:683)
	at com.google.cloud.logging.LogEntry$StructuredLogFormatter.appendField(LogEntry.java:693)
	at com.google.cloud.logging.LogEntry.toStructuredJsonString(LogEntry.java:737)
	at com.google.cloud.logging.LogEntryTest.testStructureLogPresentations(LogEntryTest.java:385)



Caused by: java.lang.reflect.InaccessibleObjectException: Unable to make field private final long java.time.Duration.seconds accessible: module java.base does not "opens java.time" to unnamed module @6996db8
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:354)
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
	at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:178)
	at java.base/java.lang.reflect.Field.setAccessible(Field.java:172)
	at com.google.gson.internal.reflect.ReflectionHelper.makeAccessible(ReflectionHelper.java:68)
	... 42 more

I'm not sure what this means: java.base does not "opens java.time"

Copy link
Contributor Author

@diegomarquezp diegomarquezp Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I believe it has something to do with modules since this behaves perfectly fine if I run the test on java 8.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok gotcha. Thanks for take a look into this.

Can you see if doing --add-opens java.base/java.time=ALL-UNNAMED would work? Something similar to https://github.com/googleapis/java-datastore/blob/8244ad258a90e5bbc7e7d8ec384555b2be84ab8f/pom.xml#L251. If this does work, I think that could be an option to consider.

Also, can you add a link for that to the PR Description as an explanation for why we are adding this.

The rest LGTM and I think we just need Cindy to review as well.

Copy link
Contributor Author

@diegomarquezp diegomarquezp Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, add-opens works but the implementation was slightly different:

  • In maven-jar-plugin, we declare a manifest entry that will be detected in Java 9+ only. This is basically the same --add-opens flag and is packed inside the jar as MANIFEST.mf
  • Since maven-surefire-plugin does not use the jar directly, I also added a surefire config to include this flag when running the tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I oversaw this brings trouble in units(8). Looking into it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we are having a limitation. The following code activates the --add-opens flag in tests when the jdk version is >= 9:

java-logging/pom.xml

Lines 305 to 317 in 5a5abd4

<activation>
<jdk>[9,)</jdk>
</activation>
<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<configuration>
<!-- This arg line allows inter-module access when Gson serializes
the private variables of java.time.Duration -->
<argLine>--add-opens java.base/java.time=ALL-UNNAMED</argLine>
</configuration>

However, when running the CIs, we specify the runtime via the -Djvm=... option (absolute path to runtime), which varies among environments - see

java-logging/.github/workflows/ci.yaml

Lines 46 to 58 in 5a5abd4

with:
java-version: 8
distribution: temurin
- name: "Set jvm system property environment variable for surefire plugin (unit tests)"
# Maven surefire plugin (unit tests) allows us to specify JVM to run the tests.
# https://maven.apache.org/surefire/maven-surefire-plugin/test-mojo.html#jvm
run: echo "SUREFIRE_JVM_OPT=-Djvm=${JAVA_HOME}/bin/java" >> $GITHUB_ENV
shell: bash
- uses: actions/setup-java@v4
with:
java-version: 17
distribution: temurin
- run: .kokoro/build.sh

and

java-logging/.kokoro/build.sh

Lines 49 to 53 in 5a5abd4

test)
echo "SUREFIRE_JVM_OPT: ${SUREFIRE_JVM_OPT}"
mvn test -B -ntp -Dclirr.skip=true -Denforcer.skip=true ${SUREFIRE_JVM_OPT}
RETURN_CODE=$?
;;

IIUC we would need a regex matcher for the <activation> entry to only include "paths not containing '1.8'".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll be reverting this maven config in the meantime


static final class SourceLocationSerializer implements JsonSerializer<SourceLocation> {
@Override
public JsonElement serialize(
Expand Down Expand Up @@ -649,6 +658,7 @@ public StructuredLogFormatter(StringBuilder builder) {
checkNotNull(builder);
this.gson =
new GsonBuilder()
.registerTypeAdapter(Duration.class, new DurationSerializer())
.registerTypeAdapter(Instant.class, new InstantSerializer())
.registerTypeAdapter(SourceLocation.class, new SourceLocationSerializer())
.registerTypeAdapter(HttpRequest.RequestMethod.class, new RequestMethodSerializer())
Expand Down
12 changes: 6 additions & 6 deletions google-cloud-logging/src/main/java/com/google/cloud/logging/testing/RemoteLoggingHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@
import com.google.cloud.logging.LoggingOptions;
import java.io.IOException;
import java.io.InputStream;
import java.time.Duration;
import java.util.UUID;
import java.util.logging.Level;
import java.util.logging.Logger;
import org.threeten.bp.Duration;

/**
* Utility to create a remote logging configuration for testing. Logging options can be obtained via
Expand Down Expand Up @@ -101,13 +101,13 @@ public static String formatForTest(String name) {

private static RetrySettings retrySettings() {
return RetrySettings.newBuilder()
.setMaxRetryDelay(Duration.ofMillis(30000L))
.setTotalTimeout(Duration.ofMillis(120000L))
.setInitialRetryDelay(Duration.ofMillis(250L))
.setMaxRetryDelayDuration(Duration.ofMillis(30000L))
.setTotalTimeoutDuration(Duration.ofMillis(120000L))
.setInitialRetryDelayDuration(Duration.ofMillis(250L))
.setRetryDelayMultiplier(1.0)
.setInitialRpcTimeout(Duration.ofMillis(120000L))
.setInitialRpcTimeoutDuration(Duration.ofMillis(120000L))
.setRpcTimeoutMultiplier(1.0)
.setMaxRpcTimeout(Duration.ofMillis(120000L))
.setMaxRpcTimeoutDuration(Duration.ofMillis(120000L))
.build();
}

Expand Down
4 changes: 2 additions & 2 deletions google-cloud-logging/src/test/java/com/google/cloud/logging/ContextTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,10 @@
import io.opentelemetry.sdk.trace.SdkTracerProvider;
import io.opentelemetry.sdk.trace.SpanProcessor;
import io.opentelemetry.sdk.trace.export.SimpleSpanProcessor;
import java.time.Duration;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
import org.threeten.bp.Duration;

@RunWith(JUnit4.class)
public class ContextTest {
Expand Down Expand Up @@ -62,7 +62,7 @@ public class ContextTest {
.setCacheHit(false)
.setCacheValidatedWithOriginServer(true)
.setCacheFillBytes(303L)
.setLatency(Duration.ofSeconds(123, 456))
.setLatencyDuration(Duration.ofSeconds(123, 456))
.build();
private static final HttpRequest PARTIAL_REQUEST =
HttpRequest.newBuilder()
Expand Down
4 changes: 2 additions & 2 deletions google-cloud-logging/src/test/java/com/google/cloud/logging/HttpRequestTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@
import static org.junit.Assert.assertTrue;

import com.google.cloud.logging.HttpRequest.RequestMethod;
import java.time.Duration;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
import org.threeten.bp.Duration;

@RunWith(JUnit4.class)
public class HttpRequestTest {
Expand Down Expand Up @@ -59,7 +59,7 @@ public class HttpRequestTest {
.setCacheHit(CACHE_HIT)
.setCacheValidatedWithOriginServer(CACHE_VALIDATED_WITH_ORIGIN_SERVER)
.setCacheFillBytes(CACHE_FILL_BYTES)
.setLatency(Duration.ofSeconds(123, 456))
.setLatencyDuration(Duration.ofSeconds(123, 456))
.build();

@Test
Expand Down
4 changes: 2 additions & 2 deletions google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingOptionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@
import com.google.api.gax.batching.FlowController;
import com.google.cloud.NoCredentials;
import com.google.cloud.TransportOptions;
import java.time.Duration;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
import org.threeten.bp.Duration;

@RunWith(JUnit4.class)
public class LoggingOptionsTest {
Expand Down Expand Up @@ -90,7 +90,7 @@ private static LoggingOptions generateLoggingOptions() {
.setIsEnabled(true)
.setElementCountThreshold(ELEMENTS_TRESHOLD_COUNT)
.setRequestByteThreshold(REQUEST_BYTE_TRESHOLD_COUNT)
.setDelayThreshold(Duration.ofMillis(DURATION))
.setDelayThresholdDuration(Duration.ofMillis(DURATION))
.setFlowControlSettings(
FlowControlSettings.newBuilder()
.setMaxOutstandingElementCount(MAX_OUTSTANDING_ELEMENTS_COUNT)
Expand Down
Loading