-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
Also add a meshed chip to the resource detail pages
There was a problem hiding this 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; |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
@dadjeibaah which meshed resources box did you mean? the octopus graph boxes? or the tables? |
@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. |
@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 :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
@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? |
There was a problem hiding this 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 Yep, that totally works for me, and sorry I didn't weigh in pre-merge. This looks good! |
@rmars same from me, incremental progress is the best of the progresses =) |
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