Skip to content

feat(big-number): add option to align time range#2

Closed
ktmud wants to merge 8 commits intobig-numberfrom
big-number-align-range
Closed

feat(big-number): add option to align time range#2
ktmud wants to merge 8 commits intobig-numberfrom
big-number-align-range

Conversation

@ktmud
Copy link
Owner

@ktmud ktmud commented Mar 16, 2020

In Superset, when a timeseries query has no data at the beginning period
or end period of the filtered time range, there will be no data records
at those periods, hence the trendline in Big Number chart will not
render them. This often causes confusion and misinterpretation
in dashboards, especially for those with multiple trendline charts
aligned with each other. They could all be a very smooth line, but
actually showing very different time ranges.

This PR adds an option "alignTimeRange" to apply the filtered time
range on the xAxis. Date periods for empty data will be rendered, but
there will be no connected lines, dots, or tooltips for them.

image

It's possible to still show tooltips for those periods, but I decided
not to do that as: 1) it's considerably more complicated to do so; 2) I don't
want to confuse zero or nulls with empty data.

🏆 Enhancements

renderTooltip: PropTypes.func,
};
const defaultProps = {
alignTimeRange: false,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is alignTimeRange required?
If both fromDateTime and toDateTime are set, would that be enough to apply the fixed domain?

What do you think if we merge fromDateTime and toDateTime fields to a single field domain: number[]?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep backward capability, I plan to make alignTimeRange disabled by default, there will be a new control in Superset that enables this behavior.

to_dttm and from_dttm (the parsed date range) is not currently available in the backend. I plan to add them as a generic API for all future charts. To keep things simple, I decided not to let the backend handle whether or not to output to_dttm and from_dttm based on chart types / control options, nor add another layer of data processing in the frontend to do so. transformProps should serve that purpose.

Not having from_dttm and to_dttm in an domain array is to follow the same coding style in the backend:
https://github.com/apache/incubator-superset/blob/53ed851339e8607f4609762664d389d60d1ad26e/superset/viz.py#L324-L352

We could have used an array variable parsed_range or something, but that adds more API for developers to remember and would make searching the code harder.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it on the date time.

I wish there is a clearer naming for alignTimeRange. align for me means adjusting the time of each data point. Perhaps fixTimeRange, useFixedTimeRange?

@kristw
Copy link

kristw commented Mar 16, 2020

Date periods for empty data will be rendered, but there will be no connected lines, dots, or tooltips for them.

Would be nice to have a tooltip showing date and value=null though. We did that for metric explorer.

@ktmud
Copy link
Owner Author

ktmud commented Mar 16, 2020

Date periods for empty data will be rendered, but there will be no connected lines, dots, or tooltips for them.

Would be nice to have a tooltip showing date and value=null though. We did that for metric explorer.

That would mean to translate time granularity to d3 ticks and reformat data values. There might also be a debate whether to do this in the frontend or backend, or some higher abstraction layer in the frontend. I'd rather push a quick fix first then iterate.

@kristw
Copy link

kristw commented Mar 16, 2020

Date periods for empty data will be rendered, but there will be no connected lines, dots, or tooltips for them.

Would be nice to have a tooltip showing date and value=null though. We did that for metric explorer.

That would mean to translate time granularity to d3 ticks and reformat data values. There might also be a debate whether to do this in the frontend or backend. I'd rather push a quick fix first then iterate.

Could you attach a screenshot of the current behavior when you hover the null data point?

@kristw
Copy link

kristw commented Mar 16, 2020

That would mean to translate time granularity to d3 ticks and reformat data values. There might also be a debate whether to do this in the frontend or backend. I'd rather push a quick fix first then iterate.

Sure. Can do in a follow-up PR. There is also existing code that may help. https://git.musta.ch/airbnb/dataportal/blob/master/dataportal/static/src/javascripts/apollo/clientResolvers/utils/fillTimeSeriesGap.ts

@ktmud
Copy link
Owner Author

ktmud commented Mar 16, 2020

Could you attach a screenshot of the current behavior when you hover the null data point?

Similar to the screenshot above. It will just be no tooltip at all.

I guess I can probably just fromDatetime and toDatetime to data.records, without getting tangled with d3.time and the ticks.

@kristw
Copy link

kristw commented Mar 16, 2020

Similar to the screenshot above. It will just be no tooltip at all.

That is fine.

I guess I can probably just fromDatetime and toDatetime to data.records, without getting tangled with d3.time and the ticks.

Sorry. I am not sure I fully understand what you mean.

@ktmud
Copy link
Owner Author

ktmud commented Mar 16, 2020

Sorry. I am not sure I fully understand what you mean.

I mean instead of fill all the missing periods with d3-time or something similar to fillTimeSeriesGap.ts, just insert fromDatetime and toDatetime to data records so that the beginning and the end of the range will have a tooltip, but anything in between will still be empty.

ktmud added 3 commits March 16, 2020 12:01
…perset#402)

This adds support for "Time Grain" in Superset for Big Number with Trendline chart.
In Superset, when a timeseries query has no data at the beginning period
or end period of the filtered time range, there will not no data records
at those periods, hence the trendline in Big Number chart would not
render those periods. This often causes confusion and misinterpretaiton
in dashboards, especially for those with multiple trendline charts
aligned with each other. They could all be a very smooth line, but
actually showing very different time ranges.

This PR adds an option "alignTimeRange" to apply the filtered time
range on the xAxis. Date periods for empty data will be rendered, but
there will be no connected lines, dots, or tooltips for them.

It's possible to still show tooltips for those periods, but I decided
not to do that as: 1) it makes things much more complicated; 2) I don't
want to confuse zero or nulls with empty data.
@ktmud ktmud force-pushed the big-number-align-range branch from 3ffef4f to 69b0ae6 Compare March 17, 2020 04:06
@ktmud ktmud force-pushed the big-number-align-range branch from 69b0ae6 to 14e7c48 Compare March 17, 2020 04:08
@ktmud
Copy link
Owner Author

ktmud commented Mar 17, 2020

Sorry. I am not sure I fully understand what you mean.

I mean instead of fill all the missing periods with d3-time or something similar to fillTimeSeriesGap.ts, just insert fromDatetime and toDatetime to data records so that the beginning and the end of the range will have a tooltip, but anything in between will still be empty.

Just realized it is not that simple as Crosshair does not support that.

@ktmud ktmud closed this Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants