fix(viz): deduce metric name if empty #16194
Conversation
Codecov Report
@@ Coverage Diff @@
## master #16194 +/- ##
==========================================
- Coverage 76.83% 76.70% -0.13%
==========================================
Files 996 996
Lines 53060 53095 +35
Branches 6766 6766
==========================================
- Hits 40766 40729 -37
- Misses 12066 12138 +72
Partials 228 228
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
villebro
force-pushed
the
villebro/implied-metric-name
branch
from
8a1f609
to
0aa628f
Compare
| :return: String representation of metric | ||
| :raises ValueError: if metric object is invalid | ||
| """ | ||
| if is_adhoc_metric(metric): |
There was a problem hiding this comment.
Could we make is_adhoc_metric a type guard so that we don't need to use cast and # type: ignore so much? It's available in typing_extensions.
There was a problem hiding this comment.
Amazing, I didn't know these were forthcoming! 🚀 Super cool, I'll implement right away and try to stay on top of new PEPs. Btw, the functionality surrounding the Metric type is is pretty bad shape (e.g. QueryObject assumes the presence of a metric type consisting of a dict with only the property label that doesn't have a type definition, some geospatial viz plugins in viz.py apparently rely on get_metric_name to return None and int values in some cases), so I decided to break up the refactor into multiple smaller PRs to keep them manageable. So I will follow-up with some additional cleanup avoid this becoming XXXL.
There was a problem hiding this comment.
FYI adding support for type guards ended up being a deep rabbit hole involving upgrading mypy, needing to change pre-commit-hooks confs and a few monitors worth of new linting errors. I'll merge this now and follow up with a PR that bumps mypy and adds support for type guards.
|
/testenv up |
|
@junlincc Ephemeral environment spinning up at http://34.221.179.62:8080. Credentials are |
|
tested locally, lgtm |
|
Ephemeral environment shutdown and build artifacts deleted. |
* fix(viz): deduce metric name if empty * fix unit test
* fix(viz): deduce metric name if empty * fix unit test
mistercrunch
added
🏷️ bot
SUMMARY
This is part 1 of a fix for #16067 to make the backend more resilient to incomplete chart metadata. If an adhoc metric is missing the
labelproperty, the backend can in most cases deduce what the label should be. Since the utilget_metric_nameis used in quite a few places in the backend and didn't have a single unit test, this PR also adds tests to ensure that the function works as expected. In addition, related types are improved and affected code updated.AFTER
BEFORE
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION