-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Math model error handling #23125
Comments
Original comment by @rashidkpc: I'm going to leave beta off of this, fixing this before beta isn't realistic. |
Original comment by @alexfrancoeur: If we're relying on SQL for aggregations and expect most of our users will want to use this Metric element to show aggregations, shouldn't we fix this experience? I guess at the end of the day it works, so I can understand the want to remove the beta label. It's just not ideal showing an error for this element type where we show a blank element for others. I guess there are bigger fish to fry. I have a feeling this will be a commonly seen bug though. |
Original comment by @cqliu1: So here's what I came across when I was testing it with It looks like the issue is the After setting the column, another issue I came across is the The last issue I ran is that the name of the column A quick fix for this is to alias the aggregation in the sql query with something like Proposed fixes:
|
Original comment by @cqliu1: Re: LINK REDACTED I'm reopening this issue. We still need to address what should happen to the args when switching datasources, like defaulting to the first available column. |
Original comment by @alexfrancoeur: @rashidkpc @cqliu1 I added a |
Original comment by @rashidkpc: The fix, eg, make it work like pointseries, is non-trivial as it's a conceptual problem. An empty table, which is what pointseries is, is just an empty table. Tables can exist without data. Single numbers are a bit different. A number can not exist without a number in it. 0 isn't right, as 0 is a valid number with meaning. For example, not knowing the number of errors is not the same as 0 errors. The fix here is to make the error dialog less jarring. There is nothing the |
Original comment by @alexfrancoeur: I'd be fine with a less jarring error that is specific to the metric vis vs. the math function. Is that something we could do before beta or still non-trivial? |
Original comment by @rashidkpc: If the requirement is that the error be specific to the metric vis, no, this is not something we could do before beta. |
Original comment by @alexfrancoeur: Honestly, I'm just looking to improve this experience in any way. I built this entire workpad using nothing but SQL and metrics from the UI last night. The experience was great with two exceptions. The first is this error. It would pop up with every metric I created. Which is fine after awhile, you learn to deal with it. But the initial experience is pretty poor, showing an error when you've done nothing wrong. The second was formatting a number, that's being tracked in LINK REDACTED I'm open to any ideas that will not surface in an error or at least a more descriptive error. If we can't fix in beta, I think this is definitely a blocker for GA. |
We already addressed the error handling and have #23111 opened for number format. Closing this out. |
Original comment by @alexfrancoeur:
I can't copy and paste the actual error, but I believe it looks like it's related to attempting to use the demo data instead of the new values. We shouldn't error out like this. Bare minimum, we should make the element empty like other elements. Ideally, we'd be more intelligent about it and use one of the new values.
The text was updated successfully, but these errors were encountered: