Skip to content
This repository has been archived by the owner on Apr 11, 2023. It is now read-only.

KIALI-529 - Graph Summary #252

Merged
merged 1 commit into from
Apr 20, 2018
Merged

Conversation

jmazzitelli
Copy link
Contributor

reorganize the summary panel graphs

@jmazzitelli jmazzitelli added the do not merge A PR is not ready to merge label Apr 14, 2018
@jmazzitelli
Copy link
Contributor Author

This isn't done yet. But its here for people to look at the code. Currently, the code has some mock data values (so I can get all four data types (2xx, 3xx, 4xx, 5xx) in the pie - so don't think the numbers will match between the pie and table. The colors in the pie are official pf palette colors. Here's what it looks like currently - see below.

Notice the pie's legend is on the bottom, not the right. I want it on the right, but putting it on the right and either the actual pie itself shrinks down really small or the legend isn't symmetrical (in two columns, 3 items on left, 1 on the right - doesn't look right). So I put the legend on the bottom. There is hovers here - hover over and you'll see tooltips showing the raw values of the slices with their percentages. Note also putting the values themselves in the legend won't look nice either - depending how long the values are, the legend will wrap or just not look good. We can try it if we think we want to do that. Standard usage of pie charts is to not put the actual values in the legend, FWIW. The tooltips gives you the values though.

I did experiment with a stacked bar graph rather than a pie graph - that might be an alternative.

screenshot from 2018-04-13 21-53-49screenshot from 2018-04-13 21-54-02

@abonas
Copy link
Contributor

abonas commented Apr 16, 2018

@serenamarie125 this is based on your feedback. wdyt?

@mwringe
Copy link
Contributor

mwringe commented Apr 16, 2018

Do we maybe want to change things like %OK to just 'Success' (and the same for 3xx, 4xx, 5xx to 'Redirections", "Client Errors", "Server Errors")? But that might take up more space than we have and make it too busy.

Do we need the '%' in there, is that not assumed because its a pie chart?

@mwringe
Copy link
Contributor

mwringe commented Apr 16, 2018

I also wonder if we could have the 'in' and 'out' side by side with one legend underneath it instead of having the same legend repeated twice.

But we need @serenamarie125 to give us the feedback on how this should be done :)

@cfcosta cfcosta changed the title Kiali 529 graph summary [WIP] KIALI-529 - Graph Summary Apr 17, 2018
@jmazzitelli jmazzitelli force-pushed the KIALI-529-graph-summary branch 2 times, most recently from 2469ead to f5b2b71 Compare April 18, 2018 17:31
@jmazzitelli
Copy link
Contributor Author

Here are some additional ideas still using pie graph but changing other things:

Pie larger size with legend on the right - legend is a single column which looks nice:

rightside-legend

Pie is smaller with the right side legend - notice the legend is forced into two columns due to the smaller size - I don't like that:

small-rightside-legend

=-=-
The rest below have the legend turned off - we still have hover tool tips to show you raw value and percentage.
=-=-

Small Pies, in a separate table but on a single row - header row says what is IN or OUT:

very-small-new-table-no-legend

One Pie per row - the left most column saying if its IN or OUT - notice the pie charts are in the "Total" column which is actually correct - the entire pie is the total, with each slice representing 2xx, 3xx, 4xx and 5xx:

very-small-no-legend

Pies on same row in same table - but the column to the left says if its IN or OUT:

very-small-same-row-no-legend

@mwringe
Copy link
Contributor

mwringe commented Apr 18, 2018

test

What about just having one legend and sharing it between the charts?

/me ducks and decides without UXD input to help clarify anything seems to be good :)

@jmazzitelli
Copy link
Contributor Author

jmazzitelli commented Apr 19, 2018

I'm fairly confident the pie component does not allow for two pie charts per legend. One legend-one chart.
We would have to have two pie charts (without the legends) and we'd create our own legend at the bottom. I can try that to see how it really looks when using the actual pie components. BTW: I do like that look the best. Hopefully, we can get the two charts-one legend to work.

BTW: since we already know we have a D3 bug screwing up our pie charts, maybe we can consider moving to another pie chart implementation? Perhaps we could get a new implementation to do things better.

@pilhuhn
Copy link
Contributor

pilhuhn commented Apr 19, 2018

Let's wait on input from @serenamarie125

@pilhuhn
Copy link
Contributor

pilhuhn commented Apr 19, 2018

@jmazzitelli If there is a bug in the PF/react pie chart, then please report it to them at https://github.com/patternfly/patternfly-react

@jmazzitelli
Copy link
Contributor Author

There already is an issue created - see: bcbcarl/react-c3js#22 and the JIRA where we discuss it here: https://issues.jboss.org/browse/KIALI-519

Copy link

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

Lots of cool things here! Trying to add comments in line

colors: { '% Success': '#0088ce', '% Fail': '#cc0000' }, // pf-blue, pf-red-100
columns: [['% Success', successRate], ['% Fail', this.props.percentError]],
type: 'pie'
colors: {

Choose a reason for hiding this comment

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

yes, let's remove the "%" from the labels

'%OK': '#0088ce', // pf-blue-400
'%3xx': '#92d400', // pf-light-green-400
'%4xx': '#ec7a08', // pf-orange-400
'%5xx': '#8b0000' // pf-red-300

Choose a reason for hiding this comment

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

should be using cc0000 // pf-red-100

type: 'pie'
colors: {
'%OK': '#0088ce', // pf-blue-400
'%3xx': '#92d400', // pf-light-green-400

Choose a reason for hiding this comment

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

should be using 3f9c35 // pf-green-400

@serenamarie125
Copy link

I did experiment with a stacked bar graph rather than a pie graph - that might be an alternative.

@jmazzitelli utilizing 100% stacked bar charts would allow a nicer visualization, IMO and then you could just have the bottom chart have a legend, which would apply to both.

What are people's thoughts about this? What's happening on hover now on these graphs ?

screen shot 2018-04-19 at 10 05 17 am

@pilhuhn
Copy link
Contributor

pilhuhn commented Apr 19, 2018 via email

@serenamarie125
Copy link

Also - chatting with Jeff Phillips, he suggests that you reference the variable itself, rather than the hex code. That way if PF colors change, no need to make a change in your code

@serenamarie125
Copy link

@pilhuhn can you help explain again the difference between those what's being shown as ok & 3xx ? then I will be able to provide an intelligent answer 😄

@pilhuhn
Copy link
Contributor

pilhuhn commented Apr 19, 2018 via email

@serenamarie125
Copy link

Also, i'm trying to understand how all the data is related ...

  1. Request Traffic includes a table & graphs. Are these associated ?
  2. What does the Incoming Request Traffic Min/Max include
  3. What does the Outgoing Request Traffic Min/Max include

I'd like to suggest a different format, but need to have a better understanding of the data and how it relates to eachother

@serenamarie125
Copy link

A 2xx is a "everything ok, here is your data" response
A 3xx is a "I don't have what you are looking for, but you can get it
over there" response

aha! So then yes i agree OK should be the green, and 3xx should be blue. We can get user feedback on this as well

@israel-hdez
Copy link
Member

@jmazzitelli utilizing 100% stacked bar charts would allow a nicer visualization, IMO and then you could just have the bottom chart have a legend, which would apply to both.

This looks quite good. And it allow us to move away from the pie chart probably solving https://issues.jboss.org/browse/KIALI-519 and https://issues.jboss.org/browse/KIALI-459.

@jmazzitelli
Copy link
Contributor Author

Here's what I got in this PR now... using stacked bar charts. Note that I have not found a way to remove the axis labels along the bottom (the number line). Not sure if we care.

screenshot from 2018-04-20 14-53-37

screenshot from 2018-04-20 14-53-51

@jmazzitelli
Copy link
Contributor Author

suggests that you reference the variable itself, rather than the hex code.

I created a separate JIRA for this - we have colors in other places we need to fix - so we'll do it all in a separate JIRA: https://issues.jboss.org/browse/KIALI-598

@jmazzitelli
Copy link
Contributor Author

I just realized something. When we used the pie chart, we were setting the percentages per slice.
Now that we are using a bar graph, I don't think we have to do this - I think we just give the raw numbers and let the graph figure out what percentage of the bar graph is what color.

Right now, notice the axis - its from 0 to 110 (the graph gives an extra 10 on the high end). But the 0..100 range is just showing up to 100% . The axis will always be 0..100. Do we care? Should we go to showing raw numbers in the bar graph?

@jmazzitelli
Copy link
Contributor Author

This is what I was afraid of with the axis datapoint labels - notice its just a jumbled mess when I just pass in the raw numbers rather than the percentages. Need to find out if the component can turn off display of that axis labels:

single:

screenshot from 2018-04-20 15-52-44

in/out:

screenshot from 2018-04-20 15-53-00

@jmazzitelli
Copy link
Contributor Author

Notice the jumbled axis labels are gone. This is about as close to @serenamarie125 's mock up as I think we can get:

screenshot from 2018-04-20 16-35-47

screenshot from 2018-04-20 16-35-57

@mtho11
Copy link
Contributor

mtho11 commented Apr 20, 2018

Looks good!

@jshaughn
Copy link
Contributor

I personally don't like the raw numbers in the chart. I think I prefer percentages such that the bars always fill the chart and 100% is the assumed x-axis max. Consider an in/out where you have a high request rate in and a low request rate out but a high % of outgoing requests fail. The out bar would be very thin because it's width is relative to the incoming rate. And the 50% failure would be represented as 1/2 that width, maybe not even visible. The code is fine but I don't like the approach. I agree with Serena's mock above, and the legend can show %, just like she has it.

@jmazzitelli
Copy link
Contributor Author

As extraordinarily surprising as it is, I believe @jshaughn is right and makes a good point. Since the in and out bars may have values that are orders of magnitude different (a single incoming request might result in 10 or more requests going out to acquire data needed to fulfill the incoming request) we are going to lose visibility into things like error rates (e.g. the red error bar might be so small you can barely see it).

I will put back the percentage values so each bar will always be full - the axis will represent 0 to 100%.

Copy link
Contributor

@mtho11 mtho11 left a comment

Choose a reason for hiding this comment

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

Easy one!

@jmazzitelli
Copy link
Contributor Author

I think I have it looking good. I put back the axis because I found how to control the ticks and labels. I will post a screenshot soon. I would say wait until Monday to merge to give others a chance to digest it.

@jmazzitelli
Copy link
Contributor Author

Bonus when doing this: the tooltips behave much better now.

It's beautify, if you ask me... like a sunset on a beach:

screenshot from 2018-04-20 17-51-17
screenshot from 2018-04-20 17-52-13

@mtho11
Copy link
Contributor

mtho11 commented Apr 20, 2018

Beautiful...ship it!

@jmazzitelli
Copy link
Contributor Author

OK, I would like others to play with this to make sure the numbers actually jive and the graph always fills to the 100% mark. It does in my small testing. If we all are in agreement this is good, it is ready to merge. I'll take off the DNM tag.

@jmazzitelli jmazzitelli removed the do not merge A PR is not ready to merge label Apr 20, 2018
@jmazzitelli jmazzitelli changed the title [WIP] KIALI-529 - Graph Summary KIALI-529 - Graph Summary Apr 20, 2018
@mtho11 mtho11 merged commit 7f4fa4c into kiali:master Apr 20, 2018
@jmazzitelli jmazzitelli deleted the KIALI-529-graph-summary branch April 20, 2018 22:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
8 participants