-
Notifications
You must be signed in to change notification settings - Fork 10
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
WIP: Metric anomaly check #103
base: master
Are you sure you want to change the base?
Conversation
val calculatedMetrics: Map[DescribedDs, Either[MetricCalculationError, Map[MetricDescriptor, MetricValue]]] = | ||
allMetricDescriptors.map { | ||
case (describedDataset, metricDescriptors) => | ||
val metricValues: Either[MetricCalculationError, Map[MetricDescriptor, MetricValue]] = | ||
MetricsCalculator.calculateMetrics(describedDataset, metricDescriptors) | ||
(describedDataset, metricValues) | ||
} | ||
val allPreviousMetricsFut: Future[Map[Instant, Map[SingleDsDescription, Map[SimpleMetricDescriptor, MetricValue]]]] = |
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.
We should load only historic metrics that we actually need
@@ -192,6 +211,7 @@ case class ChecksSuite( | |||
|
|||
for { | |||
_ <- storedMetricsFut | |||
allPreviousMetrics: Map[Instant, Map[SingleDsDescription, Map[SimpleMetricDescriptor, MetricValue]]] <- allPreviousMetricsFut |
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.
As per above we should only load historic metrics we actually need
69802f8
to
ba46206
Compare
That would be a great feature. What's left for this to get merged? |
@kingbrown9 in the current form it could be merged with not too much more work - would just need to fully review the code. The reason it stalled was because there's an issue which is trickier to solve. When you get an anomalous result should future anomaly checks take that into account or not? There's no way to know without human intervention to say if it was legitimate or not. This was part of the reason for starting to work on a UI (which is still in a very early, basic state, though did work in a simple form with the API, though it didn't cover this use case yet). Thoughts appreciated though if it's useful even in it's current form as probably not too much work to get this merged. |
No description provided.