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

Make API calls with time travel timestamp #2600

Merged
merged 11 commits into from
Jun 20, 2017

Conversation

fbarl
Copy link
Contributor

@fbarl fbarl commented Jun 13, 2017

Fixes #2609. Now we (hopefully) only need to adress #2608 before removing the feature flag.

Changes
  • Node details now display timestamps relative to the ttime we jumped to with the Time Travel control.
  • List of topologies as well as topology stats also change when travelling in time.
  • Modified the overlay displayed when jumping in time so that it affects the whole rest of the screen (with node details and topologies lists) and not just the rendered nodes.
  • Improved the empty state messaging for the resource view, as it became more important with time travelling.
Pain points
  • Would be nice to update the timestamps in Node Details every second, e.g. by wrapping them in some sort of <AutoRefreshable /> component.
  • A lot of selectors/actions logic is becoming unnecessarily complicated because Resource View is an exception to a lot of things, drawing its displayed nodes from nodesByTopology global state instead of nodes - would be nice to unify these two global states into one.

@fbarl fbarl self-assigned this Jun 13, 2017
@fbarl fbarl mentioned this pull request Jun 13, 2017
@fbarl fbarl changed the title [WIP] Add timestamp support to API endpoints [WIP] Make API calls with time travel timestamp Jun 13, 2017
@fbarl fbarl force-pushed the time-travel-add-timestamp-to-all-api-endpoints branch 4 times, most recently from 6a87b15 to 19552d6 Compare June 19, 2017 13:25
@fbarl fbarl changed the title [WIP] Make API calls with time travel timestamp Make API calls with time travel timestamp Jun 19, 2017
@fbarl fbarl requested review from 2opremio, davkal and jpellizzari and removed request for davkal June 19, 2017 16:49
@@ -415,6 +415,16 @@ type topologyStats struct {
FilteredNodes int `json:"filtered_nodes"`
}

// deserializedTimestamp converts the ISO8601 query param into a proper timestamp.
func deserializedTimestamp(timestampStr string) time.Time {

This comment was marked as abuse.

This comment was marked as abuse.

@2opremio
Copy link
Contributor

Backend code LGTM (modulo minor naming nits)

Copy link
Contributor

@jpellizzari jpellizzari left a comment

Choose a reason for hiding this comment

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

LGTM! Only one question about Go error handling. I am assuming we are going to do another iteration on the UI design for time travel? The feature itself is not very prominent, but I can save that feedback if we are doing another round of changes.

// If no timestamp is given, assume the current time.
timestamp := time.Now()
if timestampStr != "" {
timestamp, _ = time.Parse(time.RFC3339, timestampStr)

This comment was marked as abuse.

This comment was marked as abuse.

@fbarl
Copy link
Contributor Author

fbarl commented Jun 20, 2017

I am assuming we are going to do another iteration on the UI design for time travel?

@jpellizzari Yes! #2608 is the design follow-up issue, thanks for reminding me to reference it here.

@fbarl fbarl force-pushed the time-travel-add-timestamp-to-all-api-endpoints branch 3 times, most recently from e5850e7 to c65b510 Compare June 20, 2017 10:05
@fbarl fbarl force-pushed the time-travel-add-timestamp-to-all-api-endpoints branch from c65b510 to 2d8437f Compare June 20, 2017 10:30
@fbarl fbarl merged commit eb64d3f into master Jun 20, 2017
@fbarl fbarl deleted the time-travel-add-timestamp-to-all-api-endpoints branch June 20, 2017 13:12
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