[WIP] Clean up chart layout code for improved maintainability

[brylie]

This pull request changed focus to refactor a critical code path for chart layout preparation, in order to improve readability and maintainability of the code.

• technical debt

Original description

• Feature

Description

Plotly.js now allows chart legends to be configured to display horizontally. This helps when the legend takes up a lot of space in the right column, or you are trying to vertically align timeseries charts in a dashboard.

Related Tickets & Documents

Initial discussion topic:
https://discuss.redash.io/t/horizontal-legend/3869

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

[arikfr]

Maybe the legend configuration should be a dropdown with the options: None, Horizontal, Vertical? (@ranbena wdyt?)

[kravets-levko]

@brylie I know that it's WIP, but can you show examples of how it works (didn't manage to get it working on preview instance - regardless of what I select - legend does not change). Thanks!

[kravets-levko]

BTW - we have some code related to this feature (https://github.com/getredash/redash/blob/master/client/app/visualizations/chart/plotly/utils.js#L746) - it moves legend below the chart on small screens, and it also changes legend orientation (vertical when legend is on the right side, and horizontal - when legend is below. Please check if this feature works fine with your changes.

[brylie]

I know that it's WIP, but can you show examples of how it works (didn't manage to get it working on preview instance - regardless of what I select - legend does not change). Thanks!

It doesn't work yet.

FWIW, if the configuration widget would pass the options object directly to the Plotly chart instance, this would probably be working now. There seems to be some intermediary code that is mapping options to the Plotly chart configuration, perhaps for legacy reasons as mentioned in the PR comment.

My preference would be to pass chart configuration directly from the chart editor to the chart, without a mapping step.

[brylie]

After reviewing the code that @kravets-levko mentioned, I found the places where legend is created/overridden to be in visualization/chart/plotly/utils.js prepareLayout and updateLayout.

As an experiment, I manually set the legend orientation to horizontal, and it now overlaps the chart. So, on one extreme we have difficulty aligning multiple time-series charts due to variable vertical legend width, and on the other end the legend overlaps the chart. I also noted the branching logic in theupdateLayout to wrestle with various chart types, screen sizes, etc. There is at least one issue in the Plotly.js issue tracker discussing legend overlap:

plotly/plotly.js#771

It is worth noting that the prepareLayout and updateLayout functions are both called 2-4 times each time a chart configuration option changes. Might there be a way to shave off some of these ~300 lines of mostly imperative code and/or reduce the number of times these functions are evaluated?

On a philosophical note, I believe the Redash code related to chart configuration/layout should strive to be declarative, leveraging Plotly.js to handle responsiveness, etc.. To that end, it might be beneficial that the Redash chart configuration UI more closely align with the Plotly semantics, passing values directly from the UI widgets to the chart, as opposed to going through the layer(s) of imperative logic.

[kravets-levko]

@brylie A lot of our custom code (not just around Plotly, but in other places as well) was added because libraries didn't support some features we needed. Plotly issue you mentioned is a very good example: you probably notices that it's open for almost three years, and seems there is no any sufficient progress on it. But users of Redash does not know about such issues, they see that there is some bug in Redash, and we cannot wait for a years while somebody will fix it in the library. So we fix it on our side, and when one day that issue becomes fixed in the library - we remove our workarounds (actually it already happened many times). Sometimes we submit fixes to that libraries if we can fix bugs, sometimes we fork a library and fix our fork. In a case of Plotly, it's too large library to deal with its codebase.

About passing config directly to Plotly - we should always keep in mind that we need to keep backward compatibility. When people upgrade (or downgrade - it also happens) Redash - everything should keep working. And probably you noticed that code that maps chart config to Plotly config is really complicated. This means two things: 1) writing migration that will convert such config to new format and back (for downgrade) will be hard, or even impossible with data loss (downgrade); 2) some options from chart config affect several Plotly settings, also there are some dependencies between options - so if we'll simplify code that prepares data for Plotly - we will make editor's code much more complicated.

We can make a lot of philosophy on top of this, but the reality is more prosaic, and we should live with it.

[brylie]

Jeppers, I had noticed the complexity :-)

[kravets-levko]

I don't trust such tools. What do they measure? I see number 27 - what does that mean? That function has top-level if which chooses different path for pie and other chart types, and then some smaller ifs for specific settings. Yes, that function is quite large, but in fact its logic is quite simple. Also, you can try to refactor it if you want - but then please do a full regression to be sure that all features keep working.

P.S. This my comment is related to a "philosophy": indeed, we all want to work only with perfect code without any legacy, we want to use libraries that do all we want and does not have any bugs, we want users that will always use our app in the right way, etc. But it's utopia.

[brylie]

Are there any upstream bug reports in Plotly.js that relate to some of the code in prepareLayout and/or updateLayout? It might be good to reference related issues in the Redash code, to make it easier to remove if/when upstream issues are resolved.

Philosophical heuristics (like simplicity, alignment, and backwards compatibility) and pragmatics aren't in opposition. Rather, these heuristics give us some pragmatic approaches to problem-solving when faced with ambiguity and complexity.

In terms of backwards compatibility, what are your thoughts on using the plotly declarative semantics as values in the UI widgets/state, so the UI remains stable while the internal 'plumbing' is perhaps more straightforward?

[kravets-levko]

In terms of backwards compatibility, what are your thoughts on using the plotly declarative semantics as values in the UI widgets/state, so the UI remains stable while the internal 'plumbing' is perhaps more straightforward?

I'll give you few simple examples:

1. stacking feature - is not supported by plotly, so cannot be directly mapped. We have to prepare data by ourselves. Also, it's more complicated when using line/area charts, because in addition to preparing data, it requires different chart settings.
2. series - for pie and other chart types this tab has different meanings. For all chart types except of pie series are directly mapped to Plotly's series, but for pie Plotly's series are subcharts, but in UI and legend we show X values (which are corresponding segments on every pie) - because it's what users expect to see.
3. formatting - tooltip formatting is not supported by Plotly at all, so we implemented it on our own; x/y axes labels have own formats specs; we still didn't decide how to implement this, but we definitely will do it one day.

I think these 3 examples are at least one half of out custom Plotly code, that we will not remove in either case because Plotly does not (and will not) support such features; some of the rest code is configuration mapping and layout workarounds.

[brylie]

I see number 27 - what does that mean? That function has top-level if which chooses different path for pie and other chart types, and then some smaller ifs for specific settings. Yes, that function is quite large, but in fact its logic is quite simple.

Yeah, I think it means there are too many branches and depth in the function. From what I am learning, there is a guideline that says "a function should have one level of abstraction.

In this case, the prepareLayout function might be simplified to a single switch statement. Each case might check for a chart type, and run another function that prepares the layout for the given chart type. That would abstract the individual chart configuration to a lower level, while keeping the single function to prepare layout for all chart types.

[kravets-levko]

Sometimes it's better to have one larger functions than a lot of small ones, because it's easier to look at the single solid piece of code instead of trying to gather all that smaller parts each of which is completely useless itself. All recommendations are just guides, but we should always use common sense to evaluate our decisions and their consequences.

[brylie]

stacking feature

Ah, does that mean the stacking is done as individual series, or what? From what I read, Plotly has a barmode: 'stack' option for bar charts. What types of charts are using stacking in Redash?

edit: I see that the stacked area chart defines stackgroup in each of the traces.

formatting

It looks like tooltips might be formatted as part of the data series. Where can I find the tooltip formatting code in Redash?

[kravets-levko]

Stacking feature is supported for bar, line and area charts. All related code is in this file - just search for stacking.

Tooltip formatting - yes, you're totally right, we use text property to set custom tooltips for each data point, however, we had to implement some of code to generate that tooltips (it was a request from users. Another part of that request is to support similar formats for axes labels).

[brylie]

Sometimes it's better to have one larger functions than a lot of small ones, because it's easier to look at the single solid piece of code instead of trying to gather all that smaller parts each of which is completely useless itself.

In this specific case, the code is 200 lines long with several nested branches. It is difficult to understand, taking considerable concentration, scrolling, re-reading, and mental mapping of variable names. I am simply suggesting that the imperative code be abstracted in to smaller functions that compose to form this larger function.

edit: here is an example:

https://github.com/getredash/redash/blob/master/client/app/visualizations/chart/plotly/utils.js#L722-L724

[kravets-levko]

I am simply suggesting that the imperative code be abstracted in to smaller functions that compose to form this larger function.

👍 👇

Also, you can try to refactor it if you want - but then please do a full regression to be sure that all features keep working.

[brylie]

I enabled the following CodeClimate maintainability checks, primarily to shine some light on some of the more difficult to comprehend code:

Complex logic (complex-logic) - Boolean logic that may be hard to understand
Method complexity (method-complexity) - Functions or methods that may be hard to understand (Cognitive Complexity)
Nested control flow (nested-control-flow) - Deeply nested control structures like if or case

I realize these checks can seem annoying, particularly when just trying to get things done. However, it is really important to get things done with clean and understandable code.

[brylie]

I took first steps towards simplifying the prepareLayout function. The main idea was to wrap up, or abstract, units of logic into semantic, single-purpose functions.

Since CodeClimate is already enabled on this repo, lets compare its complexity scores before and after the refactor:

Before:

After:

It would probably be good for this project to adjust the CodeClimate complexity threshold, rather than disabling the check completely. There may also be a middle road by configuring CodeClimate not to block a PR from being merged -- while maintaining visibility into code quality metrics.

[brylie]

In order to prevent this pull request from getting too far behind the development branch, I would like to limit the focus to just this maintainability refactor. Then I can continue the work for horizontal legend is a follow-up task/PR.

[brylie]

The more I work through some of the functions, I am starting to realize that a considerable amount of complexity is introduced because Plotly library expects data to be 'turned inside out' in order to be fed into the axes, labels, etc.. In effect, the Plotly API leaves it to the downstream project to map over all data values, sometimes multiple times, so it can be transformed into structures that Plotly expects.

An alternative approach would be a more declarative, data-centric interface, where the chart library does the mapping and transformation internally. This style of data visualization programming can be experienced with Seaborn, Altair/Vega, and Taucharts.

I realize that it isn't practical for the project to adopt a different charting library, but just wanted to share the above realization.

[brylie]

Hei @akariv and @kravets-levko, this is about as much refactoring as I currently have bandwidth for. Please review.

[arikfr]

While I appreciate the effort and help, I'm somewhat uncomfortable with such a refactor without having some test coverage in place. 🤔

[brylie]

Sure, that makes sense. The primary changes are copy/paste into smaller functions. There is only really one function where I changed the flow of code to be more generic for multiple y-axes. I believe manual testing could cover the changes in this PR, but that automated tests should be part of the Redash development process in general.

[brylie]

To clarify, any existing tests for the main functions in plotly/utils.js should pass, in particular:

• prepareLayout
• preparePieData

If anything, this refactor should make it easier to increase test coverage, as it decomposes a couple of larger functions into smaller, single-purpose functions. This makes it more tractable to add test cases for the (smaller) function outcomes.

[arikfr]

The Plotly glue code went under refactoring as part of the migration to React and got tests (!). This is no longer relevant, but it should be easier to do any improvements to the updated code (at least it will be easier to do with more confidence).

Thanks!