Fix sending of certain metrics to backend (#3009)

aws · Nov 25, 2024 · c8f65cb · c8f65cb
1 parent 3228a5d
commit c8f65cb
Show file tree

Hide file tree

Showing 4 changed files with 202 additions and 10 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -24,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Fixed incoming audio loss calculation when server side network adaption and redundant audio features are running together.
 - Prevent DataMessage callback errors from killing a meeting
 - Do not store metrics for video send stream that no longer exists after reconnection
+- Send audio downstream decoder loss and jitter buffer size to backend as intended, for internal debugging
 
 ## [3.25.0] - 2024-09-10
 

diff --git a/demos/browser/package-lock.json b/demos/browser/package-lock.json
diff --git a/src/statscollector/StatsCollector.ts b/src/statscollector/StatsCollector.ts
@@ -422,18 +422,38 @@ export default class StatsCollector implements RedundantAudioRecoveryMetricsObse
         streamMetricReport.direction
       );
 
-      for (const metricName in streamMetricReport.currentMetrics) {
-        this.addMetricFrame(metricName, clientMetricFrame, metricMap[metricName], Number(ssrc));
+      // We loop over 'metricMap' instead of the members of 'streamMetricReport' because
+      // the latter does not include metrics (e.g. audio downstream 'decoderLoss') that are
+      // entirely dependent on other metrics (in this case 'concealedSamples' and 'totalSamplesReceived')
+      // for their calculation
+      for (const metricName in metricMap) {
+        if (this.hasMetricDependenciesInReport(metricName, streamMetricReport)) {
+          this.addMetricFrame(metricName, clientMetricFrame, metricMap[metricName], Number(ssrc));
+        }
       }
+    }
+  }
 
-      for (const metricName in streamMetricReport.currentStringMetrics) {
-        this.addMetricFrame(metricName, clientMetricFrame, metricMap[metricName], Number(ssrc));
-      }
+  private hasMetricDependenciesInReport(
+    metricName: string,
+    streamMetricReport: StreamMetricReport
+  ): boolean {
+    // Unfortunately the metrics maps do not expose their dependencies so we have to manually
+    // hard code them for metrics we know have them
+    const isSpecialMetricAndHasDependencies =
+      (metricName === 'decoderLoss' &&
+        streamMetricReport.currentMetrics['concealedSamples'] !== undefined &&
+        streamMetricReport.currentMetrics['totalSamplesReceived'] !== undefined) ||
+      (metricName === 'jitterBufferMs' &&
+        streamMetricReport.currentMetrics['jitterBufferDelay'] !== undefined &&
+        streamMetricReport.currentMetrics['jitterBufferEmittedCount'] !== undefined);
 
-      for (const metricName in streamMetricReport.currentObjectMetrics) {
-        this.addMetricFrame(metricName, clientMetricFrame, metricMap[metricName], Number(ssrc));
-      }
-    }
+    return (
+      isSpecialMetricAndHasDependencies ||
+      metricName in streamMetricReport.currentMetrics ||
+      metricName in streamMetricReport.currentStringMetrics ||
+      metricName in streamMetricReport.currentObjectMetrics
+    );
   }
 
   /**

diff --git a/test/statscollector/StatsCollector.test.ts b/test/statscollector/StatsCollector.test.ts
@@ -19,6 +19,7 @@ import MeetingSessionStatus from '../../src/meetingsession/MeetingSessionStatus'
 import MeetingSessionStatusCode from '../../src/meetingsession/MeetingSessionStatusCode';
 import TimeoutScheduler from '../../src/scheduler/TimeoutScheduler';
 import DefaultSignalingClient from '../../src/signalingclient/DefaultSignalingClient';
+import { SdkClientMetricFrame, SdkMetric } from '../../src/signalingprotocol/SignalingProtocol';
 import AudioLogEvent from '../../src/statscollector/AudioLogEvent';
 import StatsCollector from '../../src/statscollector/StatsCollector';
 import VideoLogEvent from '../../src/statscollector/VideoLogEvent';
@@ -728,6 +729,177 @@ describe('StatsCollector', () => {
         done();
       });
     });
+    it('adds decoderLoss metric when concealedSamples and totalSamplesReceived are present', done => {
+      class TestVideoStreamIndex extends DefaultVideoStreamIndex {
+        streamIdForSSRC(_ssrcId: number): number {
+          return 1;
+        }
+      }
+
+      domMockBehavior.rtcPeerConnectionGetStatsReports = [
+        {
+          id: 'RTCInboundRTPAudioStream',
+          type: 'inbound-rtp',
+          kind: 'audio',
+          ssrc: 12345,
+          concealedSamples: 500,
+          totalSamplesReceived: 1000,
+        },
+      ];
+
+      statsCollector = new StatsCollector(audioVideoController, logger, interval);
+      statsCollector.start(signalingClient, new TestVideoStreamIndex(logger));
+      const spy = sinon.spy(signalingClient, 'sendClientMetrics');
+
+      new TimeoutScheduler(interval + 5).start(() => {
+        try {
+          statsCollector.stop();
+
+          const sentMetricFrame = spy.getCall(0).args[0] as SdkClientMetricFrame;
+
+          const decoderLossMetric = sentMetricFrame.streamMetricFrames[0]?.metrics.find(
+            metric => metric.type === SdkMetric.Type.RTC_SPK_FRACTION_DECODER_LOSS_PERCENT
+          );
+
+          expect(decoderLossMetric).to.exist;
+          expect(decoderLossMetric.value).to.equal((500 / 1000) * 100);
+
+          spy.restore();
+          done();
+        } catch (error) {
+          spy.restore();
+          done(error);
+        }
+      });
+    });
+
+    it('adds jitterBufferMs metric when jitterBufferDelay and jitterBufferEmittedCount are present', done => {
+      class TestVideoStreamIndex extends DefaultVideoStreamIndex {
+        streamIdForSSRC(_ssrcId: number): number {
+          return 1;
+        }
+      }
+
+      domMockBehavior.rtcPeerConnectionGetStatsReports = [
+        {
+          id: 'RTCInboundRTPAudioStream',
+          type: 'inbound-rtp',
+          kind: 'audio',
+          ssrc: 12345,
+          jitterBufferDelay: 0.8,
+          jitterBufferEmittedCount: 200,
+        },
+      ];
+
+      statsCollector = new StatsCollector(audioVideoController, logger, interval);
+      statsCollector.start(signalingClient, new TestVideoStreamIndex(logger));
+      const spy = sinon.spy(signalingClient, 'sendClientMetrics');
+
+      new TimeoutScheduler(interval + 5).start(() => {
+        try {
+          statsCollector.stop();
+
+          const sentMetricFrame = spy.getCall(0).args[0] as SdkClientMetricFrame;
+
+          const jitterBufferMsMetric = sentMetricFrame.streamMetricFrames[0]?.metrics.find(
+            metric => metric.type === SdkMetric.Type.RTC_SPK_JITTER_BUFFER_MS
+          );
+
+          expect(jitterBufferMsMetric).to.exist;
+          expect(jitterBufferMsMetric.value).to.equal((0.8 / 200) * 1000);
+
+          spy.restore();
+          done();
+        } catch (error) {
+          spy.restore();
+          done(error);
+        }
+      });
+    });
+
+    it('does not send decoderLoss metric when dependencies are missing', done => {
+      class TestVideoStreamIndex extends DefaultVideoStreamIndex {
+        streamIdForSSRC(_ssrcId: number): number {
+          return 1;
+        }
+      }
+
+      domMockBehavior.rtcPeerConnectionGetStatsReports = [
+        {
+          id: 'RTCInboundRTPAudioStream',
+          type: 'inbound-rtp',
+          kind: 'audio',
+          ssrc: 12345,
+          totalSamplesReceived: 1000,
+        },
+      ];
+
+      statsCollector = new StatsCollector(audioVideoController, logger, interval);
+      statsCollector.start(signalingClient, new TestVideoStreamIndex(logger));
+      const spy = sinon.spy(signalingClient, 'sendClientMetrics');
+
+      new TimeoutScheduler(interval + 5).start(() => {
+        try {
+          statsCollector.stop();
+
+          const sentMetricFrame = spy.getCall(0).args[0] as SdkClientMetricFrame;
+
+          const decoderLossMetric = sentMetricFrame.streamMetricFrames[0]?.metrics.find(
+            metric => metric.type === SdkMetric.Type.RTC_SPK_FRACTION_DECODER_LOSS_PERCENT
+          );
+
+          expect(decoderLossMetric).to.not.exist;
+
+          spy.restore();
+          done();
+        } catch (error) {
+          spy.restore();
+          done(error);
+        }
+      });
+    });
+
+    it('does not send jitterBufferMs metric when dependencies are missing', done => {
+      class TestVideoStreamIndex extends DefaultVideoStreamIndex {
+        streamIdForSSRC(_ssrcId: number): number {
+          return 1;
+        }
+      }
+
+      domMockBehavior.rtcPeerConnectionGetStatsReports = [
+        {
+          id: 'RTCInboundRTPAudioStream',
+          type: 'inbound-rtp',
+          kind: 'audio',
+          ssrc: 12345,
+          jitterBufferDelay: 0.8,
+        },
+      ];
+
+      statsCollector = new StatsCollector(audioVideoController, logger, interval);
+      statsCollector.start(signalingClient, new TestVideoStreamIndex(logger));
+      const spy = sinon.spy(signalingClient, 'sendClientMetrics');
+
+      new TimeoutScheduler(interval + 5).start(() => {
+        try {
+          statsCollector.stop();
+
+          const sentMetricFrame = spy.getCall(0).args[0] as SdkClientMetricFrame;
+
+          const jitterBufferMsMetric = sentMetricFrame.streamMetricFrames[0]?.metrics.find(
+            metric => metric.type === SdkMetric.Type.RTC_SPK_JITTER_BUFFER_MS
+          );
+
+          expect(jitterBufferMsMetric).to.not.exist;
+
+          spy.restore();
+          done();
+        } catch (error) {
+          spy.restore();
+          done(error);
+        }
+      });
+    });
   });
 
   describe('stop', () => {