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

Publish is_drift metric to Prom #1263

Merged
merged 7 commits into from
Jul 3, 2023

Conversation

joshsgoldstein
Copy link
Contributor

added drift metrics to be scraped by prometheus

@adriangonz adriangonz changed the title Metrics update Expose is_drift metric to Prom Jun 30, 2023
@adriangonz adriangonz changed the title Expose is_drift metric to Prom Publish is_drift metric to Prom Jun 30, 2023
Copy link
Contributor

@ascillitoe ascillitoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two initial questions before final review.

runtimes/alibi-detect/mlserver_alibi_detect/runtime.py Outdated Show resolved Hide resolved
return

data = current_pred["data"]
if "is_drift" in data:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question for @adriangonz and @joshsgoldstein. What is the motivation for this PR?

Asking as wondering if we are definitely only concerned about is_drift here? Often p_val and distance are of equal (or greater) interest. As is threshold, and distance_threshold (dependent on detector).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ascillitoe we are enabling scv2 alerting in deploy

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially we will just expose is_drift - as that's good enough to set up alerting. I agree that exposing the other metrics will eventually become important though, but that's probably outside the scope of this PR.

Copy link
Contributor

@ascillitoe ascillitoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

(except for the failing tests?)

@adriangonz adriangonz merged commit d0187a9 into SeldonIO:master Jul 3, 2023
adriangonz pushed a commit that referenced this pull request Jul 4, 2023
Co-authored-by: Josh <josh4goldstein@gmail.com>
Co-authored-by: Adrian Gonzalez-Martin <agm@seldon.io>
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.

3 participants