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

Humanize reported durations #2915

Merged
merged 4 commits into from
Nov 6, 2017
Merged

Humanize reported durations #2915

merged 4 commits into from
Nov 6, 2017

Conversation

fbarl
Copy link
Contributor

@fbarl fbarl commented Oct 30, 2017

Resolves #2875.

image

Changes
  • Collected and briefly documented all the data types we're sending through report fields - except for string which is implicit, those data types were so far: number, datetime, ip.
  • Added a new data type duration and sending that data from the backend in a number format instead of formatted string so that the frontend can take of nice formatting.
  • Nice formatting of durations on frontend - rounding down the values to be consistent with other fields; unit tested.
Notes
  • The changes should be compatible with old probes/reports as durations would be simply sent as string and shown in the old format in that case. Merging reports should also be fine as reconciliation of those fields is simply done by rewritting (needs to be double checked still).

@fbarl fbarl self-assigned this Oct 30, 2017
@fbarl
Copy link
Contributor Author

fbarl commented Oct 30, 2017

@fbarl fbarl force-pushed the 2875-humanize-durations branch from bcd014c to 32373e3 Compare November 1, 2017 14:15
@fbarl fbarl changed the title [WIP] Humanize reported durations Humanize reported durations Nov 1, 2017
@fbarl fbarl force-pushed the 2875-humanize-durations branch from e1fa25b to cc52c98 Compare November 1, 2017 15:57
@fbarl fbarl requested a review from dlespiau November 1, 2017 16:02
@fbarl fbarl force-pushed the 2875-humanize-durations branch from 787c4cd to e233e64 Compare November 3, 2017 09:44
Copy link
Contributor

@dlespiau dlespiau left a comment

Choose a reason for hiding this comment

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

OK, spent some time in the report structures and the values in the "latest" map are (string, string). So we're actually not changing the type here, we serialise strings.

The merging code for "latest" just picks up the most recent value, no problem there either.

The display logic in the UI is what matters then, but because of the new dataType, the UI will do the right thing.

lgtm!

@@ -0,0 +1,15 @@
package report

const (

This comment was marked as abuse.

This comment was marked as abuse.

@fbarl fbarl merged commit 119bbab into master Nov 6, 2017
@dlespiau dlespiau deleted the 2875-humanize-durations branch November 21, 2017 08:49
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.

2 participants