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

Add display chips to the resource detail page to provide more detail #2063

Merged
merged 3 commits into from
Jan 17, 2019

Conversation

rmars
Copy link

@rmars rmars commented Jan 10, 2019

Trying this out: having "meshed" and "no traffic" badges on the resource detail pages to display a little more info to the user.

This branch also contains some code cleanup in ResourceDetail.jsx

screen shot 2019-01-09 at 1 19 39 pm
screen shot 2019-01-09 at 1 18 18 pm

Risha Mars added 2 commits January 9, 2019 13:20
@rmars rmars added the area/web label Jan 10, 2019
@rmars rmars self-assigned this Jan 10, 2019
Copy link
Contributor

@dadjeibaah dadjeibaah left a comment

Choose a reason for hiding this comment

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

This is s great idea @rmars, love it. I was thinking about the placement of the chips. I think having it that information appear inside the meshed resources box would be ideal, but I don't know if that is possible with what we have in the ResourceDetail page. WDYT?

@@ -21,6 +21,9 @@ import _reduce from 'lodash/reduce';
import { processNeighborData } from './util/TapUtils.jsx';
import { withContext } from './util/AppContext.jsx';

// if there has been no traffic for some time, show a warning
const showNoTrafficMsgDelay = 1000;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be good to be more explicit about how long of a duration showNoTrafficMsgDelay is. What do you thinking about changing it to showNoTrafficMsgDelayMs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, should the duration be longer? What criteria should we use to pick the right duration? Is this variable used for debugging purposes to indicate if something is wrong or is it purely informative?

Copy link
Author

Choose a reason for hiding this comment

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

whoooooops I changed it to 1 sec for testing and forgot to change it back. I think a 5-10 second delay would be more appropriate!

Copy link
Author

Choose a reason for hiding this comment

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

basically I don't want it to be flickering if there's intermittent traffic (low RPS from the service) but I do want it to appear soon enough so that the user sees it before they leave the empty page in confusion

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have an API for this? It would be nice to know that we got metrics at some point, even though they're not in the current window. Feels related-ish to #1875.

Copy link
Author

Choose a reason for hiding this comment

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

yes! same idea. it would be nice to know if we got metrics at some point. we could add this into the metrics response somehow.... or maybe put this in the get api

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree -- seems like we could introduce an API that does a historical prometheus query for this information. Trying to determine based on current traffic seems difficult, and it can lead to the flickering issue that @rmars mentioned:

screenrecording20190111at2

@rmars
Copy link
Author

rmars commented Jan 11, 2019

@dadjeibaah which meshed resources box did you mean? the octopus graph boxes? or the tables?

@dadjeibaah
Copy link
Contributor

@rmars I was referring to the octopus graph boxes, however, I do agree with you about the "flickering" issue. That could be distracting to a user. Another thing I was thinking about is if we did want to include it in the octopus graph box, we could represent both states (Meshed, Receiving traffic) with some font-awesome icon so that it is visible but not too distracting. Buuuuut, we would have to provide a legend or something to that effect so the user has an idea of what those icons represent.

siggy added a commit that referenced this pull request Jan 16, 2019
Depends on #2037, #2038, #2063, #2066, #2086, #2087, #2089

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
siggy added a commit that referenced this pull request Jan 16, 2019
Depends on #2037, #2038, #2063, #2066, #2086, #2087, #2089

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
@rmars
Copy link
Author

rmars commented Jan 16, 2019

@dadjeibaah yeah it would be cool to add some sort of visual indicator to the octopus graph boxes that would indicate "meshed" status better. Currently if a box appears with a traffic bar, that resource is meshed; if it is unmeshed but is still sending traffic, it appears in the unmeshed box. This is pretty subtle, would be better to call it out.

I think at the top level of the page it might still be good to indicate whether the resource you are viewing is meshed or not / has traffic or not so I'd still leave the top indicator (unless it gets really crowded after updating the octopus graph views).

I've updated the variable name and the delay timing in case @siggy wants to merge this PR before releasing :)

Copy link
Member

@siggy siggy left a comment

Choose a reason for hiding this comment

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

lgtm!

@rmars
Copy link
Author

rmars commented Jan 17, 2019

@dadjeibaah @klingerf @grampelberg what are your thoughts on merging this and implementing via api as a follow-up vs just waiting until we have an api?
I've raised the timeout to 6 seconds so there won't be (obvious) flickering unless the rps is that low

Copy link
Contributor

@dadjeibaah dadjeibaah left a comment

Choose a reason for hiding this comment

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

That sounds like a good plan @rmars most of my comments were about UI style but I am happy with what you have currently.

@rmars rmars merged commit 2adcdfb into master Jan 17, 2019
@rmars rmars deleted the rmars/no-traffic-msg branch January 17, 2019 21:58
@klingerf
Copy link
Contributor

@rmars Yep, that totally works for me, and sorry I didn't weigh in pre-merge. This looks good!

siggy added a commit that referenced this pull request Jan 17, 2019
Depends on #2037, #2038, #2063, #2066, #2087, #2089, #2105

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
@grampelberg
Copy link
Contributor

@rmars same from me, incremental progress is the best of the progresses =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants