-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[ML] DF Analytics should always display operational stats #54210
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
[ML] DF Analytics should always display operational stats #54210
Conversation
This commit populates the _stats API response with sensible "empty" `data_counts` and `memory_usage` objects when the job itself has not started reporting them.
|
Pinging @elastic/ml-core (:ml) |
| Map<String, DataFrameAnalyticsConfig> configById = new HashMap<>(); | ||
| for (DataFrameAnalyticsConfig config : configs) { | ||
| configById.put(config.getId(), config); | ||
| } |
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.
| Map<String, DataFrameAnalyticsConfig> configById = new HashMap<>(); | |
| for (DataFrameAnalyticsConfig config : configs) { | |
| configById.put(config.getId(), config); | |
| } | |
| Map<String, DataFrameAnalyticsConfig> configById = configs.stream().collect(Collectors.toMap(DataFrameAnalyticsConfig::getId, Function.identity())); |
| statsItem.getState(), | ||
| statsItem.getFailureReason(), | ||
| statsItem.getProgress(), | ||
| statsItem.getDataCounts() == null ? new DataCounts(config.getId()) : statsItem.getDataCounts(), |
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.
Why can't this and the memory usage be initialized in the stats ctor?
this.memoryUsage = memoryUsage == null ? new MemoryUsage(config.getId(), Instant.now(), 0) : memoryUsage;
this.dataCounts = dataCounts == null ? new DataCounts(config.getId()) : dataCounts;
Why do we need to set memory_usage create time to the config's create time? Is that adding value for the added complexity?
Or maybe I am misunderstanding the Stats#getId() value...
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.
I agree that's too much complexity for the gain. I'll make MemoryUsage.timestamp nullable.
client/rest-high-level/src/test/java/org/elasticsearch/client/MachineLearningIT.java
Outdated
Show resolved
Hide resolved
...k/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/dataframe/stats/MemoryUsage.java
Show resolved
Hide resolved
...c/main/java/org/elasticsearch/xpack/ml/action/TransportGetDataFrameAnalyticsStatsAction.java
Show resolved
Hide resolved
|
@elasticmachine update branch |
…ic#54210) This commit populates the _stats API response with sensible "empty" `data_counts` and `memory_usage` objects when the job itself has not started reporting them. Backport of elastic#54210
przemekwitek
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.
LGTM
This commit populates the _stats API response with sensible "empty"
data_countsandmemory_usageobjects when the job itselfhas not started reporting them.