Wireformat checks property as object, not array, keyed by check name

[derekm]

checks property as an array has limited use-cases for efficient retrieval of specific checks.

If I'm writing, say, a Nagios check for specific data points, I want to address directly into those data points by check name rather that iterate over the list looking for exactly the check name I'm interested in.

[derekm]

In other words, the only benefit of checks as an array is that check name becomes optional and can be non-unique.

This request implies that not specifying a check name is an error. It also opens a question about what to do if there are non-unique check names.

[heiko-braun]

If I understand you correctly, you are suggesting to turn the checks element into an object, with each check name becoming property? i.e

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

[derekm]

Yes, something like that.

Then external check scripts that hit the /health endpoint can directly address their area of concern instead of loop over the checks in a search.

[derekm]

Although, I might wonder about waiting until 1.1 to make this change.

Technically, this would be a breaking change for endpoint consumers and health checks that lacked names, so maybe a minor version increment may be deceiving.

[johnament]

You can easily do this without impacting backwards compatibility. I just pushed up this commit to hammock - hammock-project/hammock@399566b - where I add support for array and map output formats. array is the default for MP, but I may switch the default to be map. Using a configuration setting a user can switch between the two.

Note that there's a unit test checking that the new format works, as well as the TCK still passing that the old format works.

[heiko-braun]

I'd rather prefer a consistent way that works across implementations, instead of feature flags that introduce divergence. The whole point of having a spec behind this is that users can rely on uniformity across implementations.

And if there is a good argument for breaking compatibility, then let's do it.

[emattheis]

I assume #34 must be addressed before this...

[derekm]

@emattheis -- Yes, the uniqueness requested by #34 "Ensure distinct health procedure names" would be required to present the checks as a keyed object or map. This is hard to enforce outside of runtime exceptions, so maybe automatic disambiguation could take place instead via the health check classes's getClass().getName() strings.

I use Hammock, so I have this already via @johnament's "workaround."

[antoinesd]

@derekm are you ok to restart discussion on that ticket?

[derekm]

I’m not opposed. The other ticket is more far reaching and calls for more breaking changes in order to improve uniqueness checking.

This ticket doesn’t necessarily request any breaking changes, unless the default output format is changed.

There are some interesting ideas in the other ticket, but I don’t know that it needs to come first. If it came second it would only improve the situation while breaking people’s code.

This ticket could be satisfied with an optional mode and with runtime exceptions at request time (or automatic disambiguation of check names at request time).

Depends more on how spec leads want to address these requests, I figure.

I’d probably do this ticket first, like Hammock has already done. Let people experiment, and then bring in the breaking changes that improve things by being able to check for errors earlier.

[derekm]

If MP Health 2.0 is now being matured in the master branch, then we should got ahead and make the breaking change described by @heiko-braun in #110 (comment).

Since the time of his last comment, the format has presumably changed to:

{
  "status": "UP",
  "checks": {
    "first-check": {
      "status": "UP",
      "data": {
        "key": "foo",
        "foo": "bar"
      }
    },
    "second-check": {
      "status": "UP"
    }
  }
}

where outcome & state have become status & status.

One rationale given for the outcome/state to status/status rename was alignment with Spring.

This breaking change to abandon "outcome" & "state" coincidentally begins to put us in compatibility with a twice-renewed Internet-Draft RFC at the IETF called "Health Check Response Format for HTTP APIs":
https://inadarei.github.io/rfc-healthcheck/

I've contacted @inadarei, the author of the RFC, and he wants to rename the RFC's details field to adopt MP Health's checks field name:
inadarei/rfc-healthcheck#24

This puts MP Health in pretty good compatibility with the RFC already, being able to extract overall health state and check names in common.

[ivangreene]

Any movement on this issue? It will be really useful, even simply as an optionally configured feature

[xstefank]

@ivangreene unfortunately this missed the breaking change release in 3.0 because of other priorities. It can be added as an optional feature. I will implement that in our implementation for experimentation first approach.