Skip to content

Actuator /configprops shows Duration properties as { "units": [ "SECONDS", "NANOS" ] } #16539

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
Closed
wants to merge 4 commits into from
Closed

Actuator /configprops shows Duration properties as { "units": [ "SECONDS", "NANOS" ] } #16539

wants to merge 4 commits into from

Conversation

dreis2211
Copy link
Contributor

Hi,

this is an attempt to fix #16526 .

Feel free to decline the PR if you had a different solution in mind.
Cheers,
Christoph

@philwebb
Copy link
Member

Wow that was quick! Thanks @dreis2211 !

@philwebb philwebb mentioned this pull request Apr 11, 2019
Closed
@philwebb philwebb added the type: bug A general bug label Apr 11, 2019
@philwebb philwebb added this to the 2.1.x milestone Apr 11, 2019
@dreis2211
Copy link
Contributor Author

Let me fix the checkstyle issue. Sorry

snicoll
snicoll reviewed Apr 15, 2019
View reviewed changes
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

I don't know Jackson a lot but I wished there was a way to auto-detect standard serializers like this. I've added a question.

...g/springframework/boot/actuate/context/properties/ConfigurationPropertiesReportEndpoint.java
@@ -171,6 +176,7 @@ private ConfigurationBeanFactoryMetadata getBeanFactoryMetadata(
*/
protected void configureObjectMapper(ObjectMapper mapper) {
mapper.configure(SerializationFeature.FAIL_ON_EMPTY_BEANS, false);
mapper.configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false);
Copy link
Member

Choose a reason for hiding this comment

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

Is that related to the support of Duration?

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 and no - this depends on how you want the durations to be displayed. See https://github.com/FasterXML/jackson-datatype-jsr310/blob/master/src/main/java/com/fasterxml/jackson/datatype/jsr310/ser/DurationSerializer.java#L63

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@snicoll I pushed a revised version that registers a module. I've tried this actually before, but it didn't work because I called it before the serializerFactory is overwritten again in applySerializationModifier. Anyway, this should be easier now. And btw: there would be a way of calling ObjectMapper.findAndRegisterModules but that might register more than you want and is sort of expensive.

@wilkinsona wilkinsona changed the title Support Duration in /configprops endpoint Actuator /configprops shows Duration properties as { "units": [ "SECONDS", "NANOS" ] } Apr 16, 2019
@snicoll snicoll self-assigned this May 3, 2019
@snicoll
Copy link
Member

snicoll commented May 3, 2019

While trying to backport this to 2.1.x, I've noticed that JavaTimeModule is not available so I've changed something to include it only when the relevant module is on the classpath. It works on master because the new version of neo4j-ogm-api has a compile dependency on it.

I do wonder though if that's what we want to do for the endpoint. Given the endpoint is focused on @ConfigurationProperties and we have a dedicated format for Duration, Maybe we should use our converter rather than the standard Duration format.

Flagging for team attention to see what the rest of the team thinks.

@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label May 3, 2019
@wilkinsona
Copy link
Member

I don't like the idea of the actuator's output format changing depending on what's on the classpath. If we need the JavaTimeModule to always be available to produce predictable output then I think we should make it a runtime dependency of the module.

The format for a duration in application.properties is, from a user's perspective, intended only to be human readable and writable. The format in the Actuator's response is intended to both human and machine readable. As such, I think we should use a standard format for the duration, rather than our own. Assuming I've read the code correctly, this proposal will result in an ISO-8601 seconds based representation which sounds good to me.

@snicoll
Copy link
Member

snicoll commented May 6, 2019

Thanks for the feedback and good point on machine readable. Given the side effect of adding the module, I am moving this fix to 2.2.x.

@snicoll snicoll removed the for: team-attention An issue we'd like other members of the team to review label May 6, 2019
@snicoll snicoll modified the milestones: 2.1.x, 2.2.x May 6, 2019
@dreis2211
Copy link
Contributor Author

Anything I can do to help here, @snicoll ?

@snicoll
Copy link
Member

snicoll commented May 6, 2019

@dreis2211 if you want to push force an update that adds the jackson module in runtime scope, feel free to do it but I am more than happy to do that as part of the polish later today.

@dreis2211
Copy link
Contributor Author

I would be only available this evening, so feel free to do it. Highly appreciated - as usual :)

@snicoll snicoll modified the milestones: 2.2.x, 2.2.0.M4 May 21, 2019
snicoll pushed a commit that referenced this pull request May 21, 2019
@dreis2211 @snicoll
Add Duration support in /configprops endpoint
47d85bb 
See gh-16539
@snicoll snicoll closed this in 2b5632e May 21, 2019
snicoll added a commit that referenced this pull request May 21, 2019
@snicoll
Merge pull request #16539 from dreis2211
b132192 
* pr/16539:
  Polish "Add Duration support in /configprops endpoint"
  Add Duration support in /configprops endpoint
@snicoll
Copy link
Member

snicoll commented May 21, 2019

Thanks again @dreis2211!

@snicoll snicoll mentioned this pull request Jul 28, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snicoll snicoll snicoll left review comments

Assignees

@snicoll snicoll

Labels
type: bug A general bug
Projects
None yet
Milestone
2.2.0.M4
Development

Successfully merging this pull request may close these issues.

Actuator /configprops shows Duration properties as { "units": [ "SECONDS", "NANOS" ] }
4 participants
@dreis2211 @philwebb @snicoll @wilkinsona