-
Notifications
You must be signed in to change notification settings - Fork 712
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
Time travel control #2524
Time travel control #2524
Conversation
Unstructured comments:
|
f6100fe
to
6f43bd9
Compare
Thanks for your comments @davkal, they were really useful! Here are my responses:
I made one or two of changes that hopefully address that issue now. Most notably, I made the whole timestamp blink (unless we're at
Yeah, the pausing was temporarily broken at that commit but that's been fixed now.
That was a CSS bug that has now also been fixed.
Not sure how I feel about that. I chose this particular format because it's more readable, and the argument of ISO timestamp being nicer to copy & paste is a bit tricky because selection gets removed every time the value changes (which is normally every second). Are there any other arguments in favor of ISO?
Right, I made the timeline more consistent without how footer is shown. A bigger problem is where to position the component on the screen. I think the current position is not that bad, but to make it better for low resolutions, we should probably either merge it back into footer or clear up some other corner of the screen (and make sure it doesn't overlap with nodes details). Some different ideas:
That's a very good point - I made the changes.
I dropped it from 16 to 12, which I think is quite visually manageable - in any case, it's much less than what Graphana has. I don't think we have a problem with vertical space there and at the same time my impression is that it might be quite useful to go back far into the past. @2opremio what do you think?
I was feeling the same so I added a short explanation just above the slider. Ticks would make things more explicit, but timestamps take too much space to make a good use of them IMO. I would personally prefer to stick with the slider as it enables us to jump between timestamps faster and it's also a good visual indicator where we are in time.
That sounds like a good idea, but the new blinking timestamp already seems sufficiently aggressive to me :) If you disagree, maybe we can talk about how this could be done nicely.
I made the whole timestamp + clock icon into a clickable button which should hopefully make it more intuitive.
I put the button between the timestamp and the pause button (it felt right since it had to do more with the timestamp and less with the paused state). I don't see the button as super important since we get the same effect by pulling the slider to the right end, so I decided to skip the text. Hopefully the tooltip is enough to suggest what the button does.
The idea that @rade and @pidster suggested was to fade out the nodes while we're moving to a different point in time so that the diff in nodes and edges would be more visible between the states. Since we're skipping a usual loader, I also added a small spinner to the timeline control while the user is transitioning. As for animating nodes & edges, it is already being done, but before we make the layout engine deterministic, I had to chose between not using cache (bunch of stuff that didn't change gets repositioned) and using the cache (things don't move around sometimes even when they change). I decided to go with the latter because the diff is easier to observe, but I'd be happy to work on further refining the caching and/or stability of the layout engine in some other PR.
I'm still open to suggestions on what that something should be exactly, but I think that the text I inserted between the range options and the slider already removes a lot of 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.
Blocker: Add mixpanel tracking.
Otherwise, code and UX looks good to merge. LGTM
Minor line comments.
Extensions:
- query available times for travel, and grey our unavailable options
- direct editing of times, for incident response
this.updateTimestamp.bind(this), TIMELINE_DEBOUNCE_INTERVAL); | ||
} | ||
|
||
componentWillMount() { |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
|
||
componentWillUnmount() { | ||
clearInterval(this.timer); | ||
this.updateTimestamp(); |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
return ( | ||
<div className="timeline-control"> | ||
{showSliderPanel && <div className="slider-panel"> | ||
<span className="caption">Explore</span> |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
{this.renderRangeOption(sliderRanges.last90Days)} | ||
{this.renderRangeOption(sliderRanges.last1Year)} | ||
</div> | ||
<div className="column"> |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
|
||
componentWillMount() { | ||
// Force periodic re-renders to update the slider position as time goes by. | ||
this.timer = setInterval(() => { this.forceUpdate(); }, TIMELINE_SLIDER_UPDATE_INTERVAL); |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
|
||
this.setState({ millisecondsInPast }); | ||
this.debouncedUpdateTimestamp(millisecondsInPast); | ||
this.props.startWebsocketTransition(); |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
@@ -1,28 +1,23 @@ | |||
import debug from 'debug'; | |||
import find from 'lodash/find'; | |||
import { find } from 'lodash'; |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
type: ActionTypes.CLICK_RESUME_UPDATE | ||
}); | ||
// Periodically merge buffered nodes deltas until the buffer is emptied. | ||
nodesDeltaBufferUpdateTimer = setInterval( |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
import { TIMELINE_TICK_INTERVAL } from '../constants/timer'; | ||
|
||
|
||
class TopologyTimestampButton extends React.Component { |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
const timestamp = isPaused ? updatePausedAt : moment().utc().subtract(millisecondsInPast); | ||
|
||
return ( | ||
<time>{timestamp.format('MMMM Do YYYY, h:mm:ss a')} UTC</time> |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
From the feedback I got today from @2opremio and @davkal, the UX of the Time Travel component as implemented in this PR still looks far from natural and intuitive and there are quite a few pain points do be addressed. Since the PR is quite big already and it's been hanging there for a while, I will merge it as it is now so that we can test its behaviour in dev, where we'll be able to get a feeling of how bad the performance really is and how much additional backend work will be needed to achieve smooth performance. Everything will be hidden behind a feature flag which will allow uninterrupted roll-outs to prod while being able to test it on dev. Regarding the next steps, I think it would be useful for me to get a fresh perspective about the UX from @bia. Otherwise, we should probably discuss more which controls would be most important for the users and which ones would be secondary/optional (while @pidster & @rade prefer the visual sliders, @davkal & @2opremio feel the direct timestamp editing would be more useful). :) |
Resolves #1620 by adding a timeline control next to the footer in the bottom-right corner of the screen.
Currently the feature is restricted to Weave Cloud (after #2575 is merged, we could also make it work with Scope standalone if the collector service is up and running) and hidden behind a feature flag (as we want to take time experimenting on dev while not blocking roll-outs to prod) - see #2607.
Also, time travel currently only happens on the websocket channel (the node deltas), so the other individual
API
calls also need to be extended with a custom timestamp to make the whole UI correct and consistent.Notable changes
handleWebsocket
function to accept an optional timestamp parameter from the UI, which means we want to be receiving periodic reports starting from that timestamp (instead of the defaulttime.Now()
).Next steps (discussed with @2opremio)
api/topology/[topId]/[nodeId]
, disregarding the timestamp. So the API needs to be extended similarly to websockets to make the nodes details data consistent with time travel control. The same goes forapi/topology?
calls which give the info about all the available (sub)topologies as well as their stats (node/edge count, etc...). Here we need to give some thought to the UI, e.g. what happens when the topology we are currently at disappears if we go back in time and it's not currently there? - Time Travel: Use timestamp in calls to /api/topology #2609i. Whenever a request comes through query service that ends up pulling the info from S3, we might want to add it to Memcache and optionally instance local memory cache.
ii. After that, we might try predicting and pre-filling memcache with the next probable requested timestamps (e.g. near future from the current timestamp).
Some other pain points