feat(metrics): add downloadIntelligenceModelsReqResp to latency metrics #3549

maxinteger · 2024-04-19T14:53:38Z

COMPLETES SPARK-503933

This pull request addresses

Implement new downloadIntelligenceModelsReqResp latency for CA metrics

by making the following changes

Updated the types and added new utility function called accumulateLatency

Change Type

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update
Tooling change
Internal code refactor

The following scenarios where tested

< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >

I certified that

I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly

Make sure to have followed the contributing guidelines before submitting.

aws-amplify-us-east-2 · 2024-04-19T14:58:29Z

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-3549.d3m3l2kee0btzx.amplifyapp.com

torgeadelin · 2024-04-22T16:43:53Z

...ages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics-latencies.ts

+  public accumulateLatency(callback: () => Promise<unknown>, key: PreComputedLatencies) {
+    const start = performance.now();
+
+    return callback().finally(() => {
+      const currentLatency = this.precomputedLatencies.get(key) ?? 0;
+      this.saveLatency(key, currentLatency + (performance.now() - start), true);
+    });
+  }


we already support this, see saveLatency overwrite param

This a special case of the overwrite when we want to add the current value to the next one before overwrite it.

It is doable with measureLatency as well, but we need to access to the current value and I really do not want to use precomputedLatencies inside cantina. Actually precomputedLatencies should be private.

never mind, I just checked again and the saveLatency does what you described.

The overwrite name is a bit misleading, because it not just simple overwrite, but also accumulate values

measureLatency exposes the overwrite property too, so you don't have to access precomputedLatencies directly in cantina

torgeadelin

one issue

torgeadelin · 2024-04-23T10:42:43Z

...ages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics-latencies.ts

@@ -93,31 +94,31 @@ export default class CallDiagnosticLatencies extends WebexPlugin {
   * Store precomputed latency value
   * @param key - key
   * @param value - value
-   * @param overwrite - overwrite existing value or add it
+   * @param accumulate - when it true, it overwrite existing value with sum of the current value and the new measurement otherwise just store the new measurement


torgeadelin · 2024-04-23T10:47:53Z

...ages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics-latencies.ts

   * @throws
   * @returns
   */
-  public saveLatency(key: PreComputedLatencies, value: number, overwrite = true) {
-    const existingValue = overwrite ? 0 : this.precomputedLatencies.get(key) || 0;
+  public saveLatency(key: PreComputedLatencies, value: number, accumulate = true) {


I've just realised, we've changed the meaning here too.
Should I accumulate?

accumulate = true it overrides.

accumulate = false, it accumulates.

Also the default for this method should be to overwrite.

So maybe change the default value and swap the tertiary statement outcomes on line 102.

good point, I fixed all of them

torgeadelin

one small issue

torgeadelin · 2024-04-24T11:59:32Z

.../internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics-latencies.ts

+    it('should save latency correctly when accumulate is true', () => {
      assert.deepEqual(cdl.precomputedLatencies.size, 0);
-      cdl.saveLatency('internal.client.pageJMT', 10, false);
+      cdl.saveLatency('internal.client.pageJMT', 10, true);
      assert.deepEqual(cdl.precomputedLatencies.size, 1);
      assert.deepEqual(cdl.precomputedLatencies.get('internal.client.pageJMT'), 10);
    });


this doesn't seem right?
i mean it doesn't make sense now, but i think it didn't make sense before either

It looks good to me. It tests the case when there is no current value. Here we can end up with NaN when the missing value not handled correctly.

torgeadelin · 2024-04-24T12:00:54Z

.../internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics-latencies.ts

      });

      it('checks measureLatency when callBack rejects', async () => {
        const key = 'internal.client.pageJMT';
-        const overwrite = true;
+        const accumulate = true;


in order to keep the test the same as before, we should say accumulate = false, as before it was overwrite = true

torgeadelin

some more comments, but non really blocking.
also, there's a lot of linting changes, not sure if we should add those

maxinteger · 2024-04-24T13:48:25Z

@torgeadelin I think the linting is coming from a recently merged PR, there is not too many and all related to metrics plugin so I think it is OK

shnaaz · 2024-04-25T10:02:35Z

packages/@webex/internal-plugin-metrics/test/unit/spec/prelogin-metrics-batcher.ts

@@ -149,7 +150,8 @@ describe('internal-plugin-metrics', () => {
      });

      it('fails if preLoinId is not set', async () => {
-        webex.internal.newMetrics.callDiagnosticMetrics.preLoginMetricsBatcher.preLoginId = undefined;
+        webex.internal.newMetrics.callDiagnosticMetrics.preLoginMetricsBatcher.preLoginId =
+          undefined;


Why this change?

shnaaz · 2024-04-25T10:02:56Z

packages/@webex/internal-plugin-metrics/test/unit/spec/prelogin-metrics-batcher.ts

+        // avoid setting .sent timestamp
+        webex.internal.newMetrics.callDiagnosticMetrics.preLoginMetricsBatcher.prepareRequest = (
+          q
+        ) => Promise.resolve(q);


Again why has this changed

shnaaz

Are the changes in indentation intentional? Seems like some are but some are not?

shnaaz

Please check latest comment

shnaaz · 2024-04-25T11:36:46Z

...ages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics-latencies.ts

    key: PreComputedLatencies,
-    overwrite = false
+    accumulate = false


Are you planning to change definition in Cantina too, I think we should

Yes, I already have the PR for that
https://sqbu-github.cisco.com/WebExSquared/webex-web-client/pull/5653

We don't have accumulateLatency anymore right?

maxinteger · 2024-04-25T15:14:44Z

@shnaaz I just updated my branch to the latest beta and run formatting, probably these lines where not correctly formatted.

I can roll it back if you want to.

shnaaz

Just roll back couple of linter changes we discussed, otherwise looks good

issue: https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-503933

…cs (#3549)

maxinteger added the validated If the pull request is validated for automation. label Apr 19, 2024

maxinteger requested a review from a team as a code owner April 19, 2024 14:53

torgeadelin reviewed Apr 22, 2024

View reviewed changes

torgeadelin requested changes Apr 22, 2024

View reviewed changes

torgeadelin reviewed Apr 23, 2024

View reviewed changes

torgeadelin requested changes Apr 23, 2024

View reviewed changes

maxinteger force-pushed the feat/SPARK-503933-add-downloadIntelligenceModelsReqResp-latency-metric branch 2 times, most recently from e5f1347 to 41a1c80 Compare April 24, 2024 11:08

torgeadelin reviewed Apr 24, 2024

View reviewed changes

torgeadelin approved these changes Apr 24, 2024

View reviewed changes

shnaaz reviewed Apr 25, 2024

View reviewed changes

shnaaz requested changes Apr 25, 2024

View reviewed changes

shnaaz reviewed Apr 25, 2024

View reviewed changes

shnaaz approved these changes Apr 26, 2024

View reviewed changes

maxinteger added 8 commits April 26, 2024 11:28

feat(metrics): add downloadIntelligenceModelsReqResp to latency metrics

4ee7449

issue: https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-503933

fix: unit test

f3aa4a3

fix: failing unit tests after build

a18d748

chore: update unit test

9379ee0

fix: remove accumulateLatency and update measureLatency jsdocs

69ab002

fix: pr comment

973950b

fix: formatting

fda1333

fix: test

2b96813

fix: pr comment

c730d70

maxinteger force-pushed the feat/SPARK-503933-add-downloadIntelligenceModelsReqResp-latency-metric branch from 2425295 to c730d70 Compare April 26, 2024 10:30

maxinteger merged commit d39b06a into webex:beta Apr 26, 2024
2 checks passed

maxinteger deleted the feat/SPARK-503933-add-downloadIntelligenceModelsReqResp-latency-metric branch April 26, 2024 15:16

Neeraj-swarnkar pushed a commit that referenced this pull request Apr 29, 2024

feat(metrics): add downloadIntelligenceModelsReqResp to latency metri…

cfe2632

…cs (#3549)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(metrics): add downloadIntelligenceModelsReqResp to latency metrics #3549

feat(metrics): add downloadIntelligenceModelsReqResp to latency metrics #3549

maxinteger commented Apr 19, 2024

aws-amplify-us-east-2 bot commented Apr 19, 2024

torgeadelin Apr 22, 2024

maxinteger Apr 23, 2024

maxinteger Apr 23, 2024

torgeadelin Apr 23, 2024

torgeadelin left a comment

torgeadelin Apr 23, 2024

torgeadelin Apr 23, 2024

torgeadelin Apr 23, 2024

torgeadelin Apr 23, 2024

maxinteger Apr 23, 2024

torgeadelin left a comment

torgeadelin Apr 24, 2024

maxinteger Apr 24, 2024

torgeadelin Apr 24, 2024

torgeadelin left a comment

maxinteger commented Apr 24, 2024

shnaaz Apr 25, 2024

shnaaz Apr 25, 2024

shnaaz left a comment

shnaaz left a comment

shnaaz Apr 25, 2024

maxinteger Apr 25, 2024

shnaaz Apr 25, 2024

maxinteger commented Apr 25, 2024

shnaaz left a comment

feat(metrics): add downloadIntelligenceModelsReqResp to latency metrics #3549

feat(metrics): add downloadIntelligenceModelsReqResp to latency metrics #3549

Conversation

maxinteger commented Apr 19, 2024

COMPLETES SPARK-503933

This pull request addresses

by making the following changes

Change Type

The following scenarios where tested

I certified that

aws-amplify-us-east-2 bot commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torgeadelin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torgeadelin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torgeadelin left a comment

Choose a reason for hiding this comment

maxinteger commented Apr 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shnaaz left a comment

Choose a reason for hiding this comment

shnaaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxinteger commented Apr 25, 2024

shnaaz left a comment

Choose a reason for hiding this comment