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

[Model Monitoring] Add application metrics to V3IO tables and endpoints #5585

Merged
merged 33 commits into from
May 19, 2024

Conversation

jond01
Copy link
Member

@jond01 jond01 commented May 16, 2024

Implements ML-6430.

Write the application metrics to V3IO KV and TSDB.
The KV store is per model endpoint, as done for results. Each key is per application + metric name, creating a schema that Grafana can consume.
The TSDB is implemented in its destined "metrics" table.

Support reading the metrics through the existing /metrics and /metrics-values API endpoints.

The testing is through some new unit tests and an extended test part in the primary system test.

@jond01 jond01 marked this pull request as draft May 16, 2024 15:28
@jond01 jond01 marked this pull request as ready for review May 16, 2024 20:11
@jond01 jond01 requested a review from Eyal-Danieli May 19, 2024 07:53
Copy link
Member

@Eyal-Danieli Eyal-Danieli left a comment

Choose a reason for hiding this comment

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

LGTM, I didn't run the tests.
I had few suggestions but in general 2 important comments:
1 - try to add more documentation. Even 1-2 lines can be super useful to understand the importance of a certain function.
2 - TBH I still not sure why not implementing these changes on top of the V3IO Connector in this PR. As long as we are using the v3io_tsdb_reader, we have to import v3io resources into the model endpoints API file, which in other words means that it is probably breaks the CE flow + anyway we will have to "rewrite" this logic on top of the abstraction, so why not now?

if kind == mm_constants.WriterEventKind.METRIC:
# TODO : Implement the logic for writing metrics to V3IO TSDB
return
if kind == mm_schemas.WriterEventKind.METRIC:
Copy link
Member

Choose a reason for hiding this comment

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

As I wrote above, I would suggest to wrap this part in a different func as well (including the initialization of the index_cols_base.

mlrun/model_monitoring/db/v3io_tsdb_reader.py Show resolved Hide resolved
server/api/api/endpoints/model_endpoints.py Outdated Show resolved Hide resolved
@jond01
Copy link
Member Author

jond01 commented May 19, 2024

2 - TBH I still not sure why not implementing these changes on top of the V3IO Connector in this PR. As long as we are using the v3io_tsdb_reader, we have to import v3io resources into the model endpoints API file, which in other words means that it is probably breaks the CE flow + anyway we will have to "rewrite" this logic on top of the abstraction, so why not now?

  1. It should indeed move there soon. I updated the comment at the top of this module in this PR.
  2. In what way do you mean it breaks the CE at the moment?
  3. Why not now - time and priorities... the top priority now is fully supporting one permutation for our consumers (UI, QA).

@assaf758 assaf758 merged commit ffda052 into mlrun:development May 19, 2024
11 checks passed
@jond01 jond01 deleted the feature/add-metrics-v3io-tables branch May 19, 2024 14:36
rokatyy pushed a commit to rokatyy/mlrun that referenced this pull request May 28, 2024
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.

None yet

3 participants