Added JavaTimeModule for entity object mapper #267
Conversation
There was a problem hiding this comment.
It would definitely be nice if our API had strict types like OTel but given we accept Object everywhere the expectations are challenging. That being said, we can't support any possible type naturally. Supporting these sort of primitives seems ok to me though, we need to add unit tests probably using test-sets to add the dependency.
| @@ -57,6 +58,7 @@ | |||
| @SuppressWarnings("checkstyle:ConstantName") | |||
| @Deprecated | |||
| protected static final ObjectMapper mapper = new ObjectMapper() | |||
| .registerModule(new JavaTimeModule()) | |||
There was a problem hiding this comment.
I suppose we can use the autoFindModule thing that object mapper has to deal with this situation.
There was a problem hiding this comment.
Can you confirm the values are formatted correctly? I think JavaTimeModule uses ISO format IIRC, which I don't think we want in x-ray model.
There was a problem hiding this comment.
I suppose we can use the autoFindModule thing that object mapper has to deal with this situation
Yeah that would actually be better, then we don't have to add the dependency ourselves. I thought it looked too magic at first, but now I see the benefits.
Can you confirm the values are formatted correctly?
Yeah sorry forgot to mention I tested on a sample app and they work as expected.
|
@anuraaga addressed comments |
| @Test | ||
| void testTimeModuleSerialization() { | ||
| Segment seg = new SegmentImpl(AWSXRay.getGlobalRecorder(),"test"); | ||
| Duration duration = Duration.ofSeconds(1); |
There was a problem hiding this comment.
Can you do some test cases for smaller and larger values too? And also timestamps?
| @Test | ||
| void testTimestampSerialization() { | ||
| Segment seg = new SegmentImpl(AWSXRay.getGlobalRecorder(), "test"); | ||
| seg.putMetadata("date", Date.from(Instant.ofEpochSecond(1616559298))); |
There was a problem hiding this comment.
Shouldn't the module allow serializing Instant without converting to Date?
There was a problem hiding this comment.
Yeah, wasn't thinking. Changed to Instant
| seg.putMetadata("date", Date.from(Instant.ofEpochSecond(1616559298))); | ||
| String serializedSeg = seg.serialize(); | ||
|
|
||
| String expected = "{\"default\":{\"date\":1616559298000}}"; |
There was a problem hiding this comment.
I guess this is a millisecond int but usually we serialize times as second double. Is this format ok?
There was a problem hiding this comment.
Huh yeah Instant serializes to a seconds double but Date doesn't. All I found was the default behavior of Jackson is to serialize Dates as timestamps rather than ISO strings, but didn't explain why they are int instead of double: https://fasterxml.github.io/jackson-databind/javadoc/2.6/com/fasterxml/jackson/databind/SerializationFeature.html#WRITE_DATES_AS_TIMESTAMPS
I guess maybe because Dates are only accurate to the nearest millisecond? First line of Javadoc: https://docs.oracle.com/javase/8/docs/api/java/util/Date.html
There was a problem hiding this comment.
I can add a separate test for Date if you want, to indicate the int format is expected
There was a problem hiding this comment.
Yeah let's go ahead and add a test for Date as well at least for documentation. This PR didn't change it's behavior anyways so it's ok.
Description of changes:
Adds a
JavaTimeModuleto our ObjectMapper for serializing entities, so that we can supportDurationand other Time-based attributes being added to Metadata. We had a customer feature request for this@anuraaga I know the tests will fail for this with
ClassDefNotFoundErrors, I wasn't sure how to resolve this without just adding a test dependency in a bunch of modules on the newcom.fasterxml.jackson.datatype:jackson-datatype-jsr310dependency.I'm also curious to hear if you think this is even a fair expectation of the SDK, or if we should just require customers to cast their timestamps to primitives before adding it as metadata.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.