Validations for when measures with create=true have name overlaps with metrics #446
Conversation
|
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
courtneyholcomb
left a comment
courtneyholcomb
left a comment
There was a problem hiding this comment.
Nit on the description - I think we wish we could throw validation errors in both of these cases, but we can
| def validate_manifest(semantic_manifest: SemanticManifestT) -> Sequence[ValidationIssue]: # noqa: D | ||
| issues: List[ValidationIssue] = [] | ||
|
|
||
| metrics_by_name = {metric.name: metric for metric in getattr(semantic_manifest, "metrics", [])} |
There was a problem hiding this comment.
what's the reason for using getattr here instead of accessing the property directly? seems like we get better type safety if we access the property?
| ), | ||
| element_type=SemanticModelElementType.MEASURE, | ||
| ) | ||
| metric_type = getattr(metric, "type", None) |
There was a problem hiding this comment.
getattr instead of accessing the property? is this because we lost type safety on metrics_by_name?
| ) | ||
| metric_type = getattr(metric, "type", None) | ||
| if metric_type == MetricType.SIMPLE: | ||
| issues.append( |
There was a problem hiding this comment.
Am I missing the part where we check if there was create_metric: True for this measure? We should only throw these validations if that property is used!
| f"metric is not a simple metric (found type: '{MetricType.DERIVED}'). Rename the " | ||
| f"measure or the metric to resolve the collision." | ||
| ) | ||
| check_error_in_issues(error_substrings=[expected_error], issues=validation_results.errors) |
There was a problem hiding this comment.
Let's also test that these validations don't get thrown if there is a name conflict but the measure doesn't use create_metric
| ) | ||
| else: | ||
| issues.append( | ||
| ValidationError( |
There was a problem hiding this comment.
Going back to the thread on the other PR:
The validation we add to DSI should log a warning for any scenario where a measure using create_metric=True has the same name as another metric in the model (any metric type).
We actually need to make both of these warnings instead of errors, unfortunately! Reason being that it will be a breaking change if we make either of them errors since the error they would have hit wouldn't have been until query time in MF. This error would unexpectedly break their jobs and cause failures for things totally unrelated to SL.
But that means we don't need to check the type and we can consolidate to one validation.
theyostalservice
changed the base branch from
patricky/validate-time-dim-has-granularity
to
graphite-base/446
Towards #387
Description
Check out this thread for more context, but the general idea is that for measures with create_metric: true, we would like to error or warn when there's a metric with the same name somewhere and the new metric won't be created.
Checklist
changie newto create a changelog entry