-
Notifications
You must be signed in to change notification settings - Fork 294
[Model Monitoring] Fix TimescaleDB endpoint count to query correct tables (ML-11624) #9164
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] Fix TimescaleDB endpoint count to query correct tables (ML-11624) #9164
Conversation
2969be0 to
e33effb
Compare
tests/model_monitoring/db/tsdb/timescaledb/test_timescaledb_queries_aggregation.py
Outdated
Show resolved
Hide resolved
Eyal-Danieli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! 2 small comments
| # Build application filter and params | ||
| app_filter_metrics = "" | ||
| app_filter_results = "" | ||
| params: list = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| params: list = [] |
| self, | ||
| start: Optional[datetime.datetime] = None, | ||
| end: Optional[datetime.datetime] = None, | ||
| application_names: Optional[list[str]] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in L448 you check if application_names is str (and if so convert it to a list) so please adjust the type annotation accordingly
| application_names: Optional[list[str]] = None, | |
| application_names: Optional[list[str] | str] = None,, |
…bles (ML-11624) Model Monitoring App was showing "Endpoints: 0" when using TimescaleDB but correctly showed endpoints when using V3IO. The root cause was that count_processed_model_endpoints was querying the PREDICTIONS table, which doesn't have an application_name column, instead of METRICS and APP_RESULTS tables which do have this column. Changes: - Move count_processed_model_endpoints from TimescaleDBPredictionsQueries to TimescaleDBConnector - Use SQL UNION to efficiently count endpoints from both METRICS and APP_RESULTS tables in a single query - Store _tables and _pre_aggregate_manager as instance variables in connector for cross-table operations - Remove unused _count_with_application_join and _count_simple methods - Update tests to use connector fixture directly Reference: ML-11624
e33effb to
f67ad32
Compare
…eries_aggregation.py Co-authored-by: Gal Topper <[email protected]>
Summary
Fixes bug where Model Monitoring App shows "Endpoints: 0" when using TimescaleDB but correctly shows endpoints with V3IO.
Root cause:
count_processed_model_endpointswas querying the PREDICTIONS table, which doesn't have anapplication_namecolumn, instead of METRICS and APP_RESULTS tables which have this column.Changes Made
count_processed_model_endpointsfromTimescaleDBPredictionsQueriestoTimescaleDBConnector_tablesand_pre_aggregate_manageras instance variables in connector for cross-table operations_count_with_application_joinand_count_simplemethodsconnectorfixture directlyTesting
Checklist
ruff formatruff checklintingReference