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.

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

Rename 'details' to 'components' in health actuator JSON #17929

Closed
philwebb opened this issue Aug 21, 2019 · 10 comments
Closed

Rename 'details' to 'components' in health actuator JSON #17929

philwebb opened this issue Aug 21, 2019 · 10 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@philwebb
Copy link
Member

Currently we have "details" in the JSON that represents both details of a Health and components of CompositeHealth. I would like to rename the latter to components.

The catch is we need to do this in a back compatible way.

@philwebb philwebb added this to the 2.2.x milestone Aug 21, 2019
@wilkinsona wilkinsona added the type: enhancement A general enhancement label Sep 13, 2019
@philwebb philwebb self-assigned this Sep 26, 2019
philwebb added a commit that referenced this issue Sep 26, 2019
Add `ApiVersion` enum that can be injected into actuator endpoints if
they need to support more than one API revision.

Spring MVC, WebFlux and Jersey integrations now detect the API version
based on the HTTP accept header. If the request explicitly accepts a
`application/vnd.spring-boot.actuator.v` media type then the version
is set from the header. If no explicit Spring Boot media type is
accepted then the latest `ApiVersion` is assumed.

A new v3 API revision has also been introduced to allow upcoming health
endpoint format changes. By default all endpoints now consume and
can produce v3, v2 and `application/json` media types.

See gh-17929
philwebb added a commit that referenced this issue Sep 26, 2019
Update `LoggersEndpointWebIntegrationTests` to ensure that the new
v3 media type can be used.

See gh-17929
@philwebb philwebb modified the milestones: 2.2.x, 2.2.0.RC1 Sep 27, 2019
@spencergibb
Copy link
Member

spencergibb commented Sep 30, 2019

This change broke a test in spring-cloud-netflix looking for 'details'. Doesn't seem to be backward compatible. https://github.com/spring-cloud/spring-cloud-netflix/blob/40ec6066c1e82a447314ebe0b45ca34e9e7034ce/spring-cloud-netflix-hystrix/src/test/java/org/springframework/cloud/netflix/hystrix/HystrixOnlyTests.java#L86-L93

Changing "details" to "components" fixes it. I can point you to the jenkins build if you'd like.

spencergibb added a commit to spring-cloud/spring-cloud-netflix that referenced this issue Sep 30, 2019
@wilkinsona
Copy link
Member

As far as I can tell, there's no Accept header being sent in the test so it's saying that it'll happily accept anything. If you specify application/vnd.spring-boot.actuator.v2+json you should get a response with details in it rather than components.

@spencergibb
Copy link
Member

spencergibb commented Sep 30, 2019

Ok. I think of backward-compatible as the user shouldn't have to make a change. This is opt-in to backward-compatibility. There should be something in a migration guide then.

@wilkinsona
Copy link
Member

I'm not sure I agree with that. Your client was saying that it was happy to accept anything, but was actually only capable of consuming application/vnd.spring-boot.actuator.v2+json. With an appropriate accept header that expressed the client's capabilities, no change would have been necessary.

That said, there may be plenty of users that have clients that do not send an Accept header and can also only consume application/vnd.spring-boot.actuator.v2+json. IMO, this fits within Boot's goal of minor updates causing at most minor pain on upgrade. Let's see what the rest of the team thinks.

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

there may be plenty of users that have clients that do not send an Accept header and can also only consume application/vnd.spring-boot.actuator.v2+json.

This was what I was thinking as well.

@philwebb
Copy link
Member Author

I wonder how many users will have tools that actually need to the extract content from the JSON? This is a change that I feel is a tricky one to make because it's quite hard to know if users will really be effected or not. My thinking was as follows:

  • Most users are probably using only the HTTP status with tools that monitor health.
  • If the health check fails they may need to inspect the JSON, but this is probably manual so the change won't hurt.
  • If a user wants to check a specific endpoint, they can use /actuator/health/<indicator> and this will work as before.
  • For general purpose tools like Spring Boot Admin, the content type header is a good way to deal with the change.

There should be something in a migration guide then.

Indeed, we've just not written the release notes yet.

@spencergibb Do you feel like many Spring Cloud users are going to find this change disruptive? How common do you think parsing the health JSON to extract specific details is?

@spencergibb
Copy link
Member

I have no insight into how many users this might affect. I'm fine either way (making no Accept return v2 or note in migration guide). We just had a test break because of it and wanted to alert you as has been requested in the past.

@wilkinsona
Copy link
Member

Sorry, @spencergibb. I should have thanked you earlier for letting us know about the potential problem. It's much appreciated as it's prompted us to give this some extra thought.

@spencergibb
Copy link
Member

No worries. I appreciate the conversation

@philwebb
Copy link
Member Author

philwebb commented Oct 2, 2019

Hey @spencergibb, we discussed this on our weekly call and we'd like to try leaving it as it is for RC1 to see if any users complain. We're hoping that digging into the JSON content isn't that common and most people will be OK with the change. If it ends up breaking lots of users we can consider reverting it before GA.

@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Oct 2, 2019
sabareeshkkanan added a commit to sabareeshkkanan/spring-cloud-netflix that referenced this issue Dec 20, 2019
* Added symbolic link of index.adoc

* Added symbolic link of index.adoc

* set replication client filters in RefreshablePeerEurekaNodes (spring-cloud#3610)

fixes spring-cloudgh-3554

* Update SNAPSHOT to 2.2.0.M2

* Going back to snapshots

* Adding spring cloud circuitbreaker hystrix implementation

* Separated circuitbreaker auto config to its own file

* Initialize remoteRegionAppWhitelist with default value (spring-cloud#3634)

* Moves non-netflix dependencies out of bom into root pom.

fixes spring-cloudgh-3639

* Removes dependency management for okhttp3

* Moves okhttp3 dep mgmt back to bom

* Re-create connection manager on "zuul.host.*" property change (spring-cloud#3407)

* Re-create connection manager on "zuul.host.*" property change 

Fixes spring-cloud#3406

* Wrap connectionManager.shutdown() in try/catch block. Polishing

* Removes okhttp3 again after added back in older branch.

* Updating readme with note about building spring-cloud-netflix-hystrix-contract.  Fixes spring-cloud#3497

* Fromatting

* Upgrades eureka to 1.9.13 and excludes compactmap.

fixes spring-cloudgh-3636

* Update SNAPSHOT to 2.1.3.RELEASE

* Going back to snapshots

* Bumping versions to 2.1.4.BUILD-SNAPSHOT after release

* Add Spring Cloud LoadBalancer starter to Eureka starters. Fixes spring-cloudgh-3646. (spring-cloud#3647)

* Update docs. (spring-cloud#3650)

* ConditionalOnMissingBean on formBodyWrapperFilter, debugFilter… (spring-cloud#3609)

* Bumping versions

* Remove spring.provides.

* removes useless comments

* Fix RestTemplateEurekaHttpClient status update endpoint.  Fixes spring-cloud#3571 (spring-cloud#3657)

* polish

* Updates health check handler to use new StatusAggregator

* Gh 3409 turbine stream test (spring-cloud#3665)

* Add stream-test-support.

* Fix condition.

* Remove outdated workaround.

* Bumping versions

* Optimize code of eureka (spring-cloud#3660)

* Optimize code of eureka

* Merge newest code

* fix checkstyle bug

* Gh 3464 upgrade scc new (spring-cloud#3667)

* Upgrade Spring Cloud Contract version to 2.1.3.RELEASE.

* Gh 3409 turbine stream test (spring-cloud#3665)

* Add stream-test-support.

* Fix condition.

* Remove outdated workaround.

(cherry picked from commit 24f5e0b)

* Updates to use "components" rather than "details"

See spring-projects/spring-boot#17929

* Applying spring-cloud#3407 to the 2.1.x branch

* Added support for reactive service discovery

* Update SNAPSHOT to 2.2.0.M3

* Going back to snapshots

* Update SNAPSHOT to 2.2.0.RC1

* Going back to snapshots

* removing resource class from circle config

* Add property to disable spring cloud circuit breaker for hystrix

* Fix command key configuration.  Use id as command key and class as group key.

* Adding configuration metadata for spring cloud circuitbreaker

* Bumping versions

* Fix some dependencies that show up in the wrong scope

Apparently you can build on the command line but Eclipse is fussy
now and wouldn't compile these projects without explicit
dependencies.

* Also add build helper config for contract tests

* Added maven flatten plugin

* Bumping versions

* Update SNAPSHOT to 2.2.0.RC2

* Going back to snapshots

* Update configuration to use proxyBeanMethods=false.  Fixes spring-cloud#3677

* Create security.md

* Update issue templates

* Bumping versions

* Bumping versions

* Update SNAPSHOT to 2.2.0.RELEASE

* Going back to snapshots

* Bumping versions to 2.2.1.BUILD-SNAPSHOT after release

* Bumping versions

* Fix typo: clas -> class (spring-cloud#3710)

* removes .flattened-pom.xml

* ignores .flattened-pom.xml

* Gh 3718 add zoned loadbalancer instrumentation (spring-cloud#3720)

* Add instrumentation for zoned LoadBalancer.

* Add documentation.

* Fix after review.

* Fix after review.

* Update SNAPSHOT to 2.2.1.RELEASE

* Going back to snapshots

* Bumping versions to 2.2.2.BUILD-SNAPSHOT after release

Co-authored-by: Marcin Grzejszczak <marcin@grzejszczak.pl>
Co-authored-by: Yuxin Bai <LittleBaiBai@users.noreply.github.com>
Co-authored-by: Spencer Gibb <spencer@gibb.tech>
Co-authored-by: Spring Buildmaster <buildmaster@springframework.org>
Co-authored-by: Ryan Baxter <rbaxter@pivotal.io>
Co-authored-by: emilnkrastev <emilnkrastev@gmail.com>
Co-authored-by: Denys Ivano <denys.ivano@gmail.com>
Co-authored-by: Olga Maciaszek-Sharma <olga.maciaszek@gmail.com>
Co-authored-by: Rafał Żukowski <rzukow@gmail.com>
Co-authored-by: OLPMO <OLPMO@users.noreply.github.com>
Co-authored-by: Tim Ysewyn <Tim.Ysewyn@me.com>
Co-authored-by: Dave Syer <david_syer@hotmail.com>
Co-authored-by: Deepika Mohan <deepikadevidm@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants