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

Don't init http phase values #581

Closed
wants to merge 1 commit into from
Closed

Conversation

SuperQ
Copy link
Member

@SuperQ SuperQ commented Mar 7, 2020

Don't init http probe phase labels for phases we haven't observed.
Avoids exposing 0 value samples for things we haven't measured.

Fixes: #579

Signed-off-by: Ben Kochie [email protected]

@SuperQ SuperQ requested a review from brian-brazil March 7, 2020 09:20
@brian-brazil
Copy link
Contributor

I'm not sure about this, having time series not predictably present (e.g. tls might be missing if a redirect breaks) could be problematic for users to handle correctly and this is also a breaking change. There's also issues like #485 around multiple requests.

We also do this for all metrics, not just this one, as this design decision predates staleness. So it'd have to be considered more broadly. In general I view these metrics as audit logging to assist figuring out why a particular probe failed, not as something you'd do math on.

@SuperQ
Copy link
Member Author

SuperQ commented Mar 9, 2020

These specific metrics are used for math, so we should be more careful about them.

We could also move the init for these metrics later in the probe phase. That way we keep the init, but don't create them when things fail early.

@brian-brazil
Copy link
Contributor

brian-brazil commented Mar 9, 2020

probe_success and probe_ssl_earliest_cert_expiry are the only metrics intended for math, everything else is audit log as far as I'm concerned. We can't record a full log of a probe in Prometheus, but we can use metrics to help narrow down what went wrong after the fact.

If we're going to change the approach of this in the code we should design it wholesale, rather than metric by metric.

@zerkms
Copy link

zerkms commented Mar 9, 2020

To me as a metrics consumer it's awkward to see some metrics to be deliberately unreliable.

If something is exposed for debugging only purposes only - it could have had a _debug or something in its name. At this moment those numbers look like any other number in any other exporter 🤷‍♂️

@brian-brazil
Copy link
Contributor

If something is exposed for debugging only purposes only - it could have had a _debug or something in its name

The vast majority of all metrics are only for debugging, so that'd be a bit redundant :)

Don't init http probe phase labels for phases we haven't observed.
Avoids exposing 0 value samples for things we haven't measured.

Fixes: #579

Signed-off-by: Ben Kochie <[email protected]>
@SuperQ SuperQ force-pushed the superq/deinit_http_labels branch from 214018d to b2c5013 Compare March 9, 2021 10:37
@electron0zero
Copy link
Member

Q: can we close this PR in favor of #865?

@SuperQ SuperQ closed this Jan 25, 2022
@SuperQ SuperQ deleted the superq/deinit_http_labels branch January 25, 2022 12:59
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

Successfully merging this pull request may close these issues.

[suggestion] Omitting metrics if not success
4 participants