[ML] Init AD model config from JSON config #1602
Conversation
Initialise CAnomalyDetectorModelConfig with values from the JSON config file parsed by CAnomalyJobConfig. Relates to elastic#1253
Keep the Java REST integration tests happy by ensuring that the detector rules are correctly updated with changes to the filter configuration.
| const std::string& summaryCountFieldName = analysisConfig.summaryCountFieldName(); | ||
| ml::core_t::TTime bucketSpan = analysisConfig.bucketSpan(); | ||
| ml::core_t::TTime latency = analysisConfig.latency(); | ||
| bool multivariateByFields = analysisConfig.multivariateByFields(); | ||
|
|
||
| ml::model_t::ESummaryMode summaryMode{ | ||
| summaryCountFieldName.empty() ? ml::model_t::E_None : ml::model_t::E_Manual}; | ||
| ml::model::CAnomalyDetectorModelConfig modelConfig{ml::model::CAnomalyDetectorModelConfig::defaultConfig( | ||
| bucketSpan, summaryMode, summaryCountFieldName, latency, multivariateByFields)}; | ||
| modelConfig.detectionRules(ml::model::CAnomalyDetectorModelConfig::TIntDetectionRuleVecUMapCRef( | ||
| fieldConfig.detectionRules())); | ||
| analysisConfig.detectionRules())); |
There was a problem hiding this comment.
In the long term this could be encapsulated in a method of CAnomalyJobConfig, say CAnomalyJobConfig::makeModelConfig(). But since you're doing a series of PRs you can save this for the next one.
| @@ -424,6 +426,10 @@ class API_EXPORT CAnomalyJob : public CDataProcessor { | |||
| //! Object to which the output is passed | |||
| CJsonOutputWriter m_JsonOutputWriter; | |||
|
|
|||
| //! Configuration settings for the analysis parsed from | |||
| //! JSON configuration file. | |||
There was a problem hiding this comment.
It would be good to say why this is non-const. It's because it gets updated by job updates right? And of course since it's a reference that will also affect everywhere else that has a reference.
include/api/CAnomalyJobConfig.h
Outdated
| @@ -68,7 +67,7 @@ class API_EXPORT CAnomalyJobConfig { | |||
| std::string m_PartitionFieldName{}; | |||
| std::string m_ExcludeFrequent{}; | |||
| std::string m_DetectorDescription{}; | |||
| CDetectionRulesJsonParser::TDetectionRuleVec m_CustomRules{}; | |||
| std::size_t m_DetectorIndex{}; | |||
There was a problem hiding this comment.
Although probably not the best choice originally, detector index is an int everywhere else, so it's best if it's an int here too. (I think originally it was some generic "identifier" instead of an array index, and -1 indicated "unknown". That's so far in the past that -1 never exists now, but it's still probably best to keep the types consistent across the codebase.)
| @@ -234,7 +257,7 @@ class API_EXPORT CAnomalyJobConfig { | |||
|
|
|||
| std::string jobId() const { return m_JobId; } | |||
| std::string jobType() const { return m_JobType; } | |||
| const CAnalysisConfig& analysisConfig() const { return m_AnalysisConfig; } | |||
There was a problem hiding this comment.
You could provide both const and non-const accessors. In future as this class gets used more widely some places may hold a const reference to the CAnomalyJobConfig, and it's annoying for those places to have to cast away constness just to get a const reference to one of the sub-objects.
Initialise CAnomalyDetectorModelConfig with values from the JSON config file parsed by CAnomalyJobConfig. Relates to elastic#1253
Initialise CAnomalyDetectorModelConfig with values from
the JSON config file parsed by CAnomalyJobConfig.
Relates to #1253