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

Revisit the message body of health #125

Closed
Emily-Jiang opened this issue May 24, 2018 · 21 comments
Closed

Revisit the message body of health #125

Emily-Jiang opened this issue May 24, 2018 · 21 comments
Milestone

Comments

@Emily-Jiang
Copy link
Member

In the current health spec, we have

{
  "outcome": "UP",
  "checks": [
    {
      "name": "first-check",
      "state": "UP",
      "data": {
        "key": "foo",
        "foo": "bar"
      }
    },
    {
        "name": "second-check",
        "state": "UP"
    }
  ]
}

In the message it says 'outcome'. I think it should be changed to 'status', which is better understood and also match with what Spring says. In this way, in service-mesh platform, it is much easier to be communicated. Thoughts?

@kenfinnigan
Copy link

Not sure I agree that status is more understood than outcome, but if there's desire to change it then I'd say change it to something like overall-status or something else that indicates it's a rollup situation.

Also, not sure how Service Mesh plays into this, can you elaborate?

@seabaylea
Copy link

I think its desirable to users to have a consistent payload across all of their microservices - regardless of the framework (or language) that each is built in.

There isn't a significant level of consistency in the way of convention today, but certainly the most common approach is to use status for the overall result.

@Emily-Jiang
Copy link
Member Author

Emily-Jiang commented Jun 14, 2018

@cescoffier further to our discussion at EclipseCon, it is a cool thing that we can sync up in different tools and languages. Wait to see your PR and great to see you are back to Health spec again!

@Ladicek
Copy link

Ladicek commented Jun 15, 2018

If there's a consensus to rename outcome to status (and I personally don't find the arguments about consistency very convincing), would it make sense to rename state of the individual checks to status as well?

@Emily-Jiang
Copy link
Member Author

Maybe renaming state in the individual to status is even better. The top level status is the aggregation of the 2nd level of status.

@cescoffier
Copy link
Contributor

+1 to have status everywhere.

I would like to discuss the backward compatibility, as it's a breaking change. Application analysing the content of the response would need to update to the new format. We can mark in the spec the outcome and state as deprecated in the spec. The implementations can decide to provide both for now or drop the old ones. WDYT?

@cescoffier
Copy link
Contributor

Another point, should we add a version field defining the version of the model?

@pilhuhn
Copy link
Contributor

pilhuhn commented Jun 18, 2018

If we add more fields, then it could also be good to optionally allow hints for operations folks, like a short description and a url to more information.

@cescoffier
Copy link
Contributor

@pilhuhn for each procedure? or in the global report?

@pilhuhn
Copy link
Contributor

pilhuhn commented Jun 19, 2018

I would do it for each procedure and the global report. In many cases we will only see the global report, but if there are many procedures then it is expected that those parts can fail independently and thus the hints would be needed for each of the individual parts that can fail.

cescoffier added a commit to cescoffier/microprofile-health that referenced this issue Jun 21, 2018
Rename check state to status

This is related to eclipse#125
@cescoffier
Copy link
Contributor

I've opened a first PR doing the renaming: #126.

@cescoffier
Copy link
Contributor

@pilhuhn the PR mentioned in my previous comment does not include extra-metadata. I would prefer having a second PR extending the model with them. Would you be ok with this?

@pilhuhn
Copy link
Contributor

pilhuhn commented Jun 21, 2018 via email

cescoffier added a commit to cescoffier/microprofile-health that referenced this issue Jun 21, 2018
@cescoffier
Copy link
Contributor

Updated PR: #127

@Emily-Jiang
Copy link
Member Author

@cescoffier pleas also update the readme doc accordingly.

@cescoffier
Copy link
Contributor

Closed by #127

@cescoffier
Copy link
Contributor

@Emily-Jiang can you close this issue? (it seems I can't)

@cescoffier
Copy link
Contributor

@Emily-Jiang I've updated the README, did I miss something?

@Emily-Jiang
Copy link
Member Author

Thank you @cescoffier ! Looks good.

@derekm
Copy link
Contributor

derekm commented Oct 18, 2018

Can we rename this issue to "Rename outcome/state to status/status" or similar, so that it is obvious what this issue deals with?

Can we also revert this breaking change and continue this discussion until Health 2.0 is being considered?

I think service meshes should be app framework aware and should talk to each app in it's own way, whether Spring, MicroProfile, Dropwizard, Play, or home-brew stackoverflow copy/paste job.

Service meshes that aren't ping/health service style aware aren't good reasons to impose changes that break service meshes in Health 1.1!

Thanks for reconsidering this change!

@derekm
Copy link
Contributor

derekm commented Oct 19, 2018

If MP Health 2.0 is being developed in the master branch now, then instead of renaming this ticket, can we reopen this ticket to track all of the changes to the message body?

We're obviously not done revisiting the message body of MP Health.

I'd like to get #110 added in for 2.0.

I'd also like to recommend working with inadarei/rfc-healthcheck#24.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants