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

Support non-string check ID values #14

Merged
merged 1 commit into from Oct 5, 2018

Conversation

Projects
None yet
2 participants
@Limess
Contributor

Limess commented Oct 4, 2018

Previously these would result in a failed scrape as the unmarshalling failed.

@Limess Limess requested a review from Financial-Times/monitoring-aggregation Oct 4, 2018

case float64:
check.ID = strconv.FormatFloat(id, 'f', -1, 64)
default:
return errors.New("Unable to unmarshal check.id. Was not a string, or float64")

This comment has been minimized.

@lucas42

lucas42 Oct 5, 2018

Member

I'm wondering whether this should just unset the ID. We have to support checks with no ID anyway, so perhaps it'd be more friendly to ignore weird IDs, rather than erroring on them.

This comment has been minimized.

@Limess

Limess Oct 5, 2018

Contributor

I think this is only going to be in cases where it's a nested object, boolean, or array. If someone is setting that for the ID there's some severe weirdness going on. I don't think we should trust the check if it's that out from what it should be.

Of course we could change this to still send it to the validator regardless of failures in parsing the body, but I believe that would be a separate piece of work if we wanted to do it.

This comment has been minimized.

@lucas42

lucas42 Oct 5, 2018

Member

I have seen some severe weirdness in healthchecks in the past! But at least with this architecture it'll only cause a localised failure, rather than the old system where it brought down healthcheck monitoring across every system. 🙈

@lucas42

lucas42 approved these changes Oct 5, 2018

@Limess Limess merged commit 2a5d251 into master Oct 5, 2018

3 checks passed

ci/circleci: build-image Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: verify Your tests passed on CircleCI!
Details

@Limess Limess deleted the 1269/numeric-check-ids branch Oct 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment