8000 config(kayenta-core): Add KayentaSerializationConfigurationProperties by nisanharamati · Pull Request #766 · spinnaker/kayenta · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

config(kayenta-core): Add KayentaSerializationConfigurationProperties #766

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

Jump to bottom
Merged
merged 3 commits into from
Jul 21, 2020
Merged

config(kayenta-core): Add KayentaSerializationConfigurationProperties #766

merged 3 commits into from
Jul 21, 2020

Conversation

nisanharamati
Copy link
Contributor
@nisanharamati nisanharamati commented Jul 21, 2020 •
edited
Loading

A recent change to the springboot.jackson.ObjectMapper's serialization behaviour changed the way canaryDuration is serialized in canary reports. This PR adds a configuration section to control that behaviour, and sets to default to match the behaviour before the change.

Jackson 2.10+ changes the default behaviour for serializing Duration from String to Timestamp.
The behaviour is already being set explicitly for Date objects, but wasn't addressed for Duration objects.
Rather than setting this property explicitly, I added a configuration stanza to kayenta.yml to allow users to set their preferred behaviour, and defaulting to the old behaviour of encoding both Date and Duration as Strings.

Before (with the updated serialization behaviour):

"canaryDuration": 1800000

After (with writeDurationsAsTimestamps: false in kayenta.yml):

"canaryDuration": "PT30M"

You can set either Date, Duration, or both to be serialized as Integer milliseconds by setting

kayenta:
  serialization:
    writeDatesAsTimestamp: true
    writeDurationsAsTimestamp: true

in kayenta.yml

@nisanharamati
config(kayenta-core): Add KayentaSerializationConfigurationProperties
7869e7d 
Jackson 2.10+ changes the default behaviour for serializing and
Duartion from String to Timestamp.
The behaviour is already being set explcitly (twice!) for Date objects,
but wasn't addressed for Durations.
Rather than setting this property explicitly, I added a configuration
stanza to kayenta.yml to allow users to set their preferred behaviour,
defaulting to the old behaviour of encoding both Date and Duration as
Strings.
@nisanharamati nisanharamati requested a review from csanden July 21, 2020 01:47
duftler
duftler reviewed Jul 21, 2020
View reviewed changes
...re/src/main/java/com/netflix/kayenta/config/KayentaSerializationConfigurationProperties.java Outdated
@@ -0,0 +1,27 @@
/*
* Copyright 2017 Google, Inc.
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/2017 Google/2020 Netflix

...re/src/main/java/com/netflix/kayenta/config/KayentaSerializationConfigurationProperties.java Outdated

public class KayentaSerializationConfigurationProperties {

@Getter @Setter private boolean writeDatesAsTimestamps = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just use @Data on the class and drop all the @Getters and @Setters?

kayenta-core/src/test/groovy/com/netflix/kayenta/metrics/CanaryScopeSpec.groovy Outdated
@@ -58,7 +57,9 @@ class CanaryScopeSpec extends Specification {

private ObjectMapper myObjectMapper() {
ObjectMapper objectMapper = new ObjectMapper();
KayentaConfiguration.configureObjectMapperFeatures(objectMapper);
KayentaSerializationConfigurationProperties kayentaSerializationConfigurationProperties = new KayentaSerializationConfigurationProperties();
kayentaSerializationConfigurationProperties.setWriteDatesAsTimestamps(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like the property defaults to false, so does this setting have any effect?

Does the test fail if this property is set to true?

@duftler
Copy link
Collaborator
duftler commented Jul 21, 2020 •
edited
Loading

I can't see how the existing tests verify that the two fields are in fact now serialized as strings. If the goal is to prevent the public-facing contract from changing unexpectedly, I don't think the tests actually do that.

The CanaryScopeSpec tests pass whether the 2 new properties are set to true or false. This is because the "should render as json and come back again" test makes an assertion over the round-tripped CanaryScope pojo. So what we are verifying is that we can deserialize the json no matter whether the dates/durations are serialized as strings or as timestamps.

I think CanaryScopeSpec could use an additional test verifying that dates/durations are serialized based on the values of the 2 new properties. Once that test is in place, we are no longer susceptible to the external behavior changing without us knowing about it.

@nisanharamati
Copy link
Contributor Author

I can't see how the existing tests verify that the two fields are in fact now serialized as strings. If the goal is to prevent the public-facing contract from changing unexpectedly, I don't think the tests actually do that.

The CanaryScopeSpec tests pass whether the 2 new properties are set to true or false. This is because the "should render as json and come back again" test makes an assertion over the round-tripped CanaryScope pojo. So what we are verifying is that we can deserialize the json no matter whether the dates/durations are serialized as strings or as timestamps.

I think CanaryScopeSpec could use an additional test verifying that dates/durations are serialized based on the values of the 2 new properties. Once that test is in place, we are no longer susceptible to the external behavior changing without us knowing about it.

I agree. I'll make it its own test outside of CanaryScopeTest, as I don't think the serialization configuration is strictly tied to the CanaryScope

@nisanharamati nisanharamati requested a review from duftler July 21, 2020 21:16
duftler
duftler approved these changes Jul 21, 2020
View reviewed changes
Copy link
Collaborator
@duftler duftler left a comment

Choose a reason for hiding this comment

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

Very nice. LGTM.

csanden
csanden approved these changes Jul 21, 2020
View reviewed changes
Copy link
Contributor
@csanden csanden left a comment

Choose a reason for hiding this comment

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

LGTM

@csanden csanden merged commit b68a5ac into spinnaker:master Jul 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csanden csanden csanden approved these changes

@duftler duftler duftler approved these changes

Assignees
No one assigned
Labels
target-release/1.22
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nisanharamati @duftler @csanden @spinnakerbot
0