From 6cf93ea480f0598b250a6312ec651c1f66a9a9f8 Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Thu, 16 May 2024 14:01:21 -0400 Subject: [PATCH] MM-58281 Allow client metrics to be floats and round timestamps (#27027) * MM-58281 Allow client metrics to be floats and round timestamps * MM-58281 Fix report version * Ensure reports can contain a single timestamp * Round timestamps in unit tests --- server/channels/app/metrics.go | 18 ++++++------- server/public/model/metrics.go | 4 +-- .../performance_telemetry/reporter.test.ts | 12 ++++----- .../utils/performance_telemetry/reporter.ts | 25 +++++++++++++++---- 4 files changed, 37 insertions(+), 22 deletions(-) diff --git a/server/channels/app/metrics.go b/server/channels/app/metrics.go index a0838ac17e..8f5b28f44a 100644 --- a/server/channels/app/metrics.go +++ b/server/channels/app/metrics.go @@ -18,7 +18,7 @@ func (a *App) RegisterPerformanceReport(rctx request.CTX, report *model.Performa for _, c := range report.Counters { switch c.Metric { case model.ClientLongTasks: - a.Metrics().IncrementClientLongTasks(commonLabels["platform"], commonLabels["agent"], float64(c.Value)) + a.Metrics().IncrementClientLongTasks(commonLabels["platform"], commonLabels["agent"], c.Value) default: // we intentionally skip unknown metrics } @@ -27,21 +27,21 @@ func (a *App) RegisterPerformanceReport(rctx request.CTX, report *model.Performa for _, h := range report.Histograms { switch h.Metric { case model.ClientTimeToFirstByte: - a.Metrics().ObserveClientTimeToFirstByte(commonLabels["platform"], commonLabels["agent"], float64(h.Value)) + a.Metrics().ObserveClientTimeToFirstByte(commonLabels["platform"], commonLabels["agent"], h.Value) case model.ClientFirstContentfulPaint: - a.Metrics().ObserveClientFirstContentfulPaint(commonLabels["platform"], commonLabels["agent"], float64(h.Value)) + a.Metrics().ObserveClientFirstContentfulPaint(commonLabels["platform"], commonLabels["agent"], h.Value) case model.ClientLargestContentfulPaint: - a.Metrics().ObserveClientLargestContentfulPaint(commonLabels["platform"], commonLabels["agent"], float64(h.Value)) + a.Metrics().ObserveClientLargestContentfulPaint(commonLabels["platform"], commonLabels["agent"], h.Value) case model.ClientInteractionToNextPaint: - a.Metrics().ObserveClientInteractionToNextPaint(commonLabels["platform"], commonLabels["agent"], float64(h.Value)) + a.Metrics().ObserveClientInteractionToNextPaint(commonLabels["platform"], commonLabels["agent"], h.Value) case model.ClientCumulativeLayoutShift: - a.Metrics().ObserveClientCumulativeLayoutShift(commonLabels["platform"], commonLabels["agent"], float64(h.Value)) + a.Metrics().ObserveClientCumulativeLayoutShift(commonLabels["platform"], commonLabels["agent"], h.Value) case model.ClientChannelSwitchDuration: - a.Metrics().ObserveClientChannelSwitchDuration(commonLabels["platform"], commonLabels["agent"], float64(h.Value)) + a.Metrics().ObserveClientChannelSwitchDuration(commonLabels["platform"], commonLabels["agent"], h.Value) case model.ClientTeamSwitchDuration: - a.Metrics().ObserveClientTeamSwitchDuration(commonLabels["platform"], commonLabels["agent"], float64(h.Value)) + a.Metrics().ObserveClientTeamSwitchDuration(commonLabels["platform"], commonLabels["agent"], h.Value) case model.ClientRHSLoadDuration: - a.Metrics().ObserveClientRHSLoadDuration(commonLabels["platform"], commonLabels["agent"], float64(h.Value)) + a.Metrics().ObserveClientRHSLoadDuration(commonLabels["platform"], commonLabels["agent"], h.Value) default: // we intentionally skip unknown metrics } diff --git a/server/public/model/metrics.go b/server/public/model/metrics.go index cea7a7f4a2..6a63c131e8 100644 --- a/server/public/model/metrics.go +++ b/server/public/model/metrics.go @@ -35,7 +35,7 @@ var ( type MetricSample struct { Metric MetricType `json:"metric"` - Value int64 `json:"value"` + Value float64 `json:"value"` Timestamp int64 `json:"timestamp,omitempty"` Labels map[string]string `json:"labels,omitempty"` } @@ -65,7 +65,7 @@ func (r *PerformanceReport) IsValid() error { return fmt.Errorf("report version is not supported: server version: %s, report version: %s", performanceReportVersion.String(), r.Version) } - if r.Start >= r.End { + if r.Start > r.End { return fmt.Errorf("report timestamps are erroneous") } diff --git a/webapp/channels/src/utils/performance_telemetry/reporter.test.ts b/webapp/channels/src/utils/performance_telemetry/reporter.test.ts index da3813d931..1b0cdfe9b7 100644 --- a/webapp/channels/src/utils/performance_telemetry/reporter.test.ts +++ b/webapp/channels/src/utils/performance_telemetry/reporter.test.ts @@ -52,23 +52,23 @@ describe('PerformanceReporter', () => { expect(sendBeacon.mock.calls[0][0]).toEqual(siteUrl + '/api/v4/client_perf'); const report = JSON.parse(sendBeacon.mock.calls[0][1]); expect(report).toMatchObject({ - start: performance.timeOrigin + testMarkA.startTime, - end: performance.timeOrigin + testMarkB.startTime, + start: Math.round(performance.timeOrigin + testMarkA.startTime), + end: Math.round(performance.timeOrigin + testMarkB.startTime), histograms: [ { metric: 'testMeasureA', value: testMarkB.startTime - testMarkA.startTime, - timestamp: performance.timeOrigin + testMarkA.startTime, + timestamp: Math.round(performance.timeOrigin + testMarkA.startTime), }, { metric: 'testMeasureB', value: testMarkC.startTime - testMarkA.startTime, - timestamp: performance.timeOrigin + testMarkA.startTime, + timestamp: Math.round(performance.timeOrigin + testMarkA.startTime), }, { metric: 'testMeasureC', value: testMarkC.startTime - testMarkB.startTime, - timestamp: performance.timeOrigin + testMarkB.startTime, + timestamp: Math.round(performance.timeOrigin + testMarkB.startTime), }, ], }); @@ -94,7 +94,7 @@ describe('PerformanceReporter', () => { expect(reporter.handleObservations).toHaveBeenCalled(); - const timestamp = performance.timeOrigin + performance.now(); + const timestamp = Math.round(performance.timeOrigin + performance.now()); await waitForReport(); diff --git a/webapp/channels/src/utils/performance_telemetry/reporter.ts b/webapp/channels/src/utils/performance_telemetry/reporter.ts index 134e234fbc..4bb9edddcf 100644 --- a/webapp/channels/src/utils/performance_telemetry/reporter.ts +++ b/webapp/channels/src/utils/performance_telemetry/reporter.ts @@ -17,13 +17,28 @@ import type {PlatformLabel, UserAgentLabel} from './platform_detection'; import {getPlatformLabel, getUserAgentLabel} from './platform_detection'; type PerformanceReportMeasure = { + + /** + * metric is the name of a counter or histogram metric which must match a MetricType constant as defined in + * model/metrics.go on the server. + */ metric: string; + + /** + * value is the floating point value of the metric. It's often a millisecond duration, but it's meaning depends + * on which metric this is. + */ value: number; + + /** + * timestamp is an integer value representing when the metric was measured as a millisecond value. Some browsers + * use floating point numbers for performance timestamps, so we need to make sure to round this. + */ timestamp: number; } type PerformanceReport = { - version: '1.0'; + version: '0.1.0'; labels: { platform: PlatformLabel; @@ -126,7 +141,7 @@ export default class PerformanceReporter { this.histogramMeasures.push({ metric: entry.name, value: entry.duration, - timestamp: performance.timeOrigin + entry.startTime, + timestamp: Math.round(performance.timeOrigin + entry.startTime), }); } @@ -151,7 +166,7 @@ export default class PerformanceReporter { this.histogramMeasures.push({ metric: metric.name, value: metric.value, - timestamp: performance.timeOrigin + performance.now(), + timestamp: Math.round(performance.timeOrigin + performance.now()), }); } @@ -212,7 +227,7 @@ export default class PerformanceReporter { const counterMeasures = this.countersToMeasures(now, counters); return { - version: '1.0', + version: '0.1.0', labels: { platform: this.platformLabel, @@ -252,7 +267,7 @@ export default class PerformanceReporter { counterMeasures.push({ metric: name, value, - timestamp: now, + timestamp: Math.round(now), }); }