Skip to content
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

Feature: Display the verbose name for metrics within Charts and legend. #3437

Closed

Conversation

fabianmenges
Copy link
Contributor

@fabianmenges fabianmenges commented Sep 8, 2017

Currently Verbose names are not shown in chart legends or tooltips if there are any groupbys, this addresses these issues and should work for all other visualizations as well as the json export, csv export, etc, as well. The names are formatted right after the queries are run.

This also allows us to get rid of the verbose name replacing all over the java script.

The problem is that some of the name formatting is currently done on the app-tier (python, e.g. joining metric name and group by name, pivoting data...) and some of it is done in Javascript (e.g. replacing verbose name).

I think we need should do all of it in one location. Either we need to return the data fairly raw to the client side and format it there (which I think would be cleaner but much more work given the current design). Or we have to do more formatting on the app-tier.

Before:
before_verbose

After:
after_verbose

@coveralls
Copy link

coveralls commented Sep 8, 2017

Coverage Status

Coverage increased (+0.02%) to 69.141% when pulling ba3f6fab7ea6487894b6f08ebfd3967ee12cc2f0 on tc-dc:fmenges/verbose_names_charts into 3dfdde1 on apache:master.

@fabianmenges fabianmenges force-pushed the fmenges/verbose_names_charts branch from ba3f6fa to 6525225 Compare September 12, 2017 14:25
@coveralls
Copy link

coveralls commented Sep 12, 2017

Coverage Status

Coverage increased (+0.02%) to 69.141% when pulling 6525225 on tc-dc:fmenges/verbose_names_charts into 147c12d on apache:master.

@mistercrunch
Copy link
Member

I'm afraid that the verbose name can have funky characters that could be problematic (is unicode in dict keys ok in 2.7?) and can't provide the guarantees we need around it being unique within a datasource (column collision in dataframes), plus often we use the coalesce of verbose_name/actual_name. I think I'd rather replace it in the view as we display things.

Of course the downside is having to do it in every single visualization.

I'm not 100% on this but I think I'd rather go in the direction of enforcing unicity in the (non-verbose) names and not allowing funky characters in there and use those internally until the view layer.

@fabianmenges
Copy link
Contributor Author

Interesting, I'll write some unit tests for the unicode character problem and check it. I can always B64 encode the dict keys.
We can pass down flags if and how to use the verbose name to python, that way visualizations that want to do something more complex with the names can do whatever they want.
In general if feel like if users go through the trouble of manually specifying verbose names they want them to be used.

I'm not super worried about name collisions since you have to manually set verbose names anyways and it should be pretty clear to anyone who can setup a datasource that creating verbose names with collision is problematic and I don't think there is a use case for doing that.
Regardless its obviously something we can test for on data entry: "verbose name" needs to be unique and they are not allowed to collide with any of the column names.

We could try to go a different route and move logic like this into JS and the data object that the explore_json endpoint returns would have a complex Json object as series_title. However that only moves the problem to JS and doesn't really solve it.

@mistercrunch
Copy link
Member

I'm not sure if you've seen how we do it in the JS in places like here?
https://github.com/apache/incubator-superset/blob/master/superset/assets/visualizations/table.js#L45

It's pretty accessible, we kind of just need to sprinkle it around all the visualizations. We could also make a utility function that's more generic and does the coalesce. Call it something like slice.verbose(s). Assuming we'd have that function and use it everywhere in the visualizations, would that cover everything you're thinking about?

@fabianmenges
Copy link
Contributor Author

Cross referencing #3504, I reimplemented the same logic in JS but for time series charts only. One of the two should work :P

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.

3 participants