Skip to content

Publish is_drift metric to Prom#1263

Merged
adriangonz merged 7 commits into
SeldonIO:masterfrom
joshsgoldstein:metrics_update
Jul 3, 2023
Merged

Publish is_drift metric to Prom#1263
adriangonz merged 7 commits into
SeldonIO:masterfrom
joshsgoldstein:metrics_update

Conversation

@joshsgoldstein

Copy link
Copy Markdown
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

@ascillitoe ascillitoe left a comment

Copy link
Copy Markdown

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.

Comment thread runtimes/alibi-detect/mlserver_alibi_detect/runtime.py Outdated
return

data = current_pred["data"]
if "is_drift" in data:

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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.

@ascillitoe ascillitoe left a comment

Copy link
Copy Markdown

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 <[email protected]>
Co-authored-by: Adrian Gonzalez-Martin <[email protected]>
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