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

Animate sparklines #795

Merged
merged 7 commits into from
Feb 5, 2016
Merged

Animate sparklines #795

merged 7 commits into from
Feb 5, 2016

Conversation

davkal
Copy link
Contributor

@davkal davkal commented Jan 5, 2016

  • Refactored sparklines to be rendered by react
  • Animate sparklines by feeding data bit by bit using a buffer
  • Corrected row key for sparklines so root element does not get replaced all the time
  • Tried using CSS transitions to animate the line between consecutive data feeds, but that still looked choppy

Fixes #638

@tomwilkie
Copy link
Contributor

Looking good

yijibh1klw

this.x.domain([first, last]);

this.y.domain([
this.props.min || d3.min(data, d => d.value),

This comment was marked as abuse.

This comment was marked as abuse.

@foot
Copy link
Contributor

foot commented Jan 5, 2016

It would be cool to have a better domain for the load averages. (They still "jump around a bit"). But I'm not sure what that would be, some dynamic rolling average perhaps.

@foot
Copy link
Contributor

foot commented Jan 5, 2016

As the sparkline updates more frequently than the label the two feel a bit out of sync sometimes. You see a spike whizz by, but the label failed to capture it. It might be a bit annoying to have the number changing too often also though.

@foot
Copy link
Contributor

foot commented Jan 5, 2016

We could separate out the logic a little more so that ticking/buffer-merging happened in an AnimatedSparkline, but that might get in the way again if we wanted to have a SmoothScrollingAnimatedSparkline in the future too..

Anywho, looks good!

@davkal
Copy link
Contributor Author

davkal commented Jan 6, 2016

The latest value next to the sparkline was out of sync because it was not rendered by the bit-by-bit fed sparkline. The logic to feed the data has been extracted but this needs to be adapted when #752 is merged, because the number layout changes there. I suggest we wait for that.

BONUS: smooth out the y-domain, maybe via an exponentially dampened moving average like load itself.

@davkal davkal changed the title Animate sparklines [WIP] Animate sparklines Jan 6, 2016
* Refactored sparklines to be rendered by react

* Correct row key for sparklines

* Extracted data feed into animated-sparkline

* last value is rendered by sparkline now, because it relies on the
  last value that it is fed, not the lastest availble value
also added sortBy date after merge in value buffer
@davkal davkal force-pushed the 638-animate-sparklines branch from 29e781f to 0a0179a Compare February 2, 2016 17:07
@davkal davkal changed the title [WIP] Animate sparklines Animate sparklines Feb 3, 2016
@davkal
Copy link
Contributor Author

davkal commented Feb 3, 2016

Format renderer fixes are included, fixes #902

@foot
Copy link
Contributor

foot commented Feb 3, 2016

Looks good!

Minor issues:

  • If you get used to the animating sparklines its a surprise to see them sit there statically for 5s when the details panel is first opened. (cheat?: Initial "load 10", and stream in the next 5 for immediate animation?)
  • Load jumping around: Maybe we could record the greatest value we've seen so far and use that as the max. Its fun to watch though, sometimes looks like a never ending staircase.
  • Top memory render gently "bumps" the layout when changing values sometimes. E.g. 302MB -> 302.1MB will make the container a tiny bit bigger to fit in the "1".

@foot
Copy link
Contributor

foot commented Feb 3, 2016

Code looks good.

HOC FTW LGTM

* also increase sparkline feed to 60 secs
* and keep historic max
* tried activating it for child tables too, but that became quite CPU
  intensive, and feed intervals became out of sync (host CPU < container
  CPU, which is hard to believe)
@davkal
Copy link
Contributor Author

davkal commented Feb 4, 2016

Final comment: the child tables are not fed by the feeder, so the big top metrics seem to change all the time, while the metrics in the tables stay the same til the next details request. This comes with 2 drawbacks: (1) they dont look like they re updating that often. I tried using the feeder there too, but it was quite CPU heavy and they quickly became out of sync. (2) as the big metric is going through the samples it may have a lower value than that of a child metric, which wont add up, e.g. host CPU < container CPU.

@foot
Copy link
Contributor

foot commented Feb 4, 2016

LGTMTM

davkal added a commit that referenced this pull request Feb 5, 2016
@davkal davkal merged commit 188eea8 into master Feb 5, 2016
@davkal davkal deleted the 638-animate-sparklines branch February 5, 2016 13:48
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