[telemetry] cmd/gotelemetry: fix chart x-axis time interval display
Hyang-Ah Hana Kim (Gerrit)
Hyang-Ah Hana Kim has uploaded the change for review![]( "Open in Gerrit")
Commit message
cmd/gotelemetry: fix chart x-axis time interval display
Align the x-axis interval to match the report interval,
by setting the observablehq plot library's interval option
to be the upload date's weekday.
https://observablehq.com/plot/features/intervals#timeInterval
And use the time format string that specifies the time
zone, to avoid misinterpretation in Javascript.
The Week field of telemetry.Report is meant to be
the start of the week period. But, uploader and
the gotelemetry view's active counter aggregation
had been using the field to store the end of
the week period.
Update the comment of telemetry.Report to match
how we use the field.
In chart drawing, it is easier to work with the
start day of each week. The report with week `w`
to the half-open `[w-7days, w)` period.
Presenting the data with the bin that corresponds
to [w-7days, w) is more compelling than mapping
it to [w, w+7days). So, use w-7days as the Week
field of the datum (the data entries for charts).
Fixes golang/go#68206
Change diff
Change information
- M cmd/gotelemetry/internal/view/view.go
- M cmd/gotelemetry/internal/view/view_test.go
- M internal/content/gotelemetryview/index.ts
- M internal/content/gotelemetryview/static/index.min.js
- M internal/content/gotelemetryview/static/index.min.js.map
- M internal/telemetry/types.go
- M internal/upload/reports.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hyang-Ah Hana Kim (Gerrit)
Hyang-Ah Hana Kim added 1 comment![]( "Open in Gerrit")
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Findley (Gerrit)
Robert Findley added 4 comments![]( "Open in Gerrit")
return t.Format("2006-01-02T15:04:05Z")Can we use 2006-01-02? It looks like that was the format used previously, and since the field name is 'DateRange', it seems to make more sense that we do not format a time component.
EDIT: ah I see the comment below about JS using local time. Perhaps that comment belongs here (or at least a short comment redirecting to the larger explanation below).
startTime, _ := parseReportDate(start)
endTime, _ := parseReportDate(end)I don't understand the behavior of falling back to timeNow when there is an error parsing the date. It seems better to surface the parse error. Perhaps we could filter out reports that have a malformed start or end date (how would that happen), collecting errors and listing errors in the view UI.
intervalEnd, err := parseReportDate(r.Week)Call this weekEnd?
log.Printf("ignoring report with invalid week date: %v", err)
continueAh, if we're ignoring the report here then we should probably also ignore it in the domain calculation. As mentioned above it might make sense to collect and surface these errors.
Related details
- Hyang-Ah Hana Kim
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hyang-Ah Hana Kim (Gerrit)
Hyang-Ah Hana Kim uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Hyang-Ah Hana Kim
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hyang-Ah Hana Kim (Gerrit)
Hyang-Ah Hana Kim uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Hyang-Ah Hana Kim
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hyang-Ah Hana Kim (Gerrit)
Hyang-Ah Hana Kim added 4 comments![]( "Open in Gerrit")
Can we use 2006-01-02? It looks like that was the format used previously, and since the field name is 'DateRange', it seems to make more sense that we do not format a time component.
EDIT: ah I see the comment below about JS using local time. Perhaps that comment belongs here (or at least a short comment redirecting to the larger explanation below).
Added comments to explain why.
startTime, _ := parseReportDate(start)
endTime, _ := parseReportDate(end)I don't understand the behavior of falling back to timeNow when there is an error parsing the date. It seems better to surface the parse error. Perhaps we could filter out reports that have a malformed start or end date (how would that happen), collecting errors and listing errors in the view UI.
The reasoning was that when this happens, there is no valid report/data to plot in `reports`, and I thought still showing an empty graph with x-axis may look better.
Now change the code to parse/check the time early on when creating `telemetryReport`.
intervalEnd, err := parseReportDate(r.Week)Hyang-Ah Hana KimCall this weekEnd?
Acknowledged
log.Printf("ignoring report with invalid week date: %v", err)
continueAh, if we're ignoring the report here then we should probably also ignore it in the domain calculation. As mentioned above it might make sense to collect and surface these errors.
now `pending` and `newTelemetryReport` are going to log and drop the reports with unexpected time string.
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |