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

feat(cc-metrics): add MetricsManager #4127

Draft
wants to merge 1 commit into
base: wxcc
Choose a base branch
from

Conversation

bhabalan
Copy link
Contributor

@bhabalan bhabalan commented Mar 3, 2025

COMPLETES #< INSERT LINK TO ISSUE >

This pull request addresses SPARK-572789

Add behavioral and operational metrics for cc sdk

by making the following changes

Added new MetricsManager class

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 were tested

  • Emitted a behavioral event and verified it in Amplitude

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.

@bhabalan bhabalan added the validated If the pull request is validated for automation. label Mar 3, 2025
@bhabalan bhabalan self-assigned this Mar 3, 2025
Copy link

coderabbitai bot commented Mar 3, 2025

📝 Walkthrough

Walkthrough

This change replaces the dependency on @webex/internal-plugin-mercury with @webex/internal-plugin-metrics in the @webex/plugin-cc package. A new MetricsManager class is introduced to centralize metrics tracking, employing a singleton pattern and providing methods for submitting both behavioral and operational events. The ContactCenter implementation now initializes a MetricsManager instance, which is integrated into the agent login process to track an 'agent-login' event. The error handling within the agent routing function has been updated to log metrics upon failures. Additionally, the module’s typing is revised to remove the old metrics property in favor of a newMetrics property with clearly defined methods for metrics submission. Corresponding unit tests have been added and modified to validate these changes across the metrics tracking functionalities.

Possibly related PRs

Warning

There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

yarn install v1.22.22
[1/4] Resolving packages...
warning eslint@8.57.1: This version is no longer supported. Please see https://eslint.org/version-support for other options.
warning eslint > @humanwhocodes/config-array@0.13.0: Use @eslint/config-array instead
warning eslint > @humanwhocodes/config-array > @humanwhocodes/object-schema@2.0.3: Use @eslint/object-schema instead
warning eslint > file-entry-cache > flat-cache > rimraf@3.0.2: Rimraf versions prior to v4 are no longer supported
warning eslint > file-entry-cache > flat-cache > rimraf > glob@7.2.3: Glob versions prior to v9 are no longer supported
warning eslint > file-entry-cache > flat-cache > rimraf > glob > inflight@1.0.6: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
warning eslint-import-resolver-typescript > glob@7.2.3: Glob versions prior to v9 are no longer supported
warning glob@7.2.3: Glob versions prior to v9 are no longer supported
warning intern > glob@7.1.7: Glob versions prior to v9 are no longer supported
warning intern > glob > inflight@1.0.6: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
warning jasmine > glob@7.2.3: Glob versions prior to v9 are no longer supported
warning jest > @jest/core > jest-runtime > glob@7.2.3: Glob versions prior to v9 are no longer supported
warning jest > jest-cli > jest-config > glob@7.2.3: Glob versions prior to v9 are no longer supported
warning jest > @jest/core > @jest/reporters > glob@7.2.3: Glob versions prior to v9 are no longer supported
warning jest > @jest/core > @jest/transform > babel-plugin-istanbul > test-exclude > glob@7.2.3: Glob versions prior to v9 are no longer supported
warning mocha > glob@7.2.0: Glob versions prior to v9 are no longer supported
warning mocha > glob > inflight@1.0.6: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
warning sinon@9.2.4: 16.1.1
warning sinon > @sinonjs/samsam > lodash.get@4.4.2: This package is deprecated. Use the optional chaining (?.) operator instead.
warning wd > request@2.88.0: request has been deprecated, see request/request#3142
warning wd > archiver > glob@7.2.3: Glob versions prior to v9 are no longer supported
warning wd > q@1.5.1: You or someone you depend on is using Q, the JavaScript Promise library that gave JavaScript developers strong feelings about promises. They can almost certainly migrate to the native JavaScript promise now. Thank you literally everyone for joining me in this bet against the odds. Be excellent to each other.

(For a CapTP with native promises, see @endo/eventual-send and @endo/captp)
warning wd > request > uuid@3.4.0: Please upgrade to version 7 or higher. Older versions may use Math.random() in certain circumstances, which is known to be problematic. See https://v8.dev/blog/math-random for details.
warning wd > request > har-validator@5.1.5: this library is no longer supported
warning wd > archiver > archiver-utils > glob@7.2.3: Glob versions prior to v9 are no longer supported
warning @babel/cli > glob@7.2.3: Glob versions prior to v9 are no longer supported
warning @babel/plugin-proposal-async-generator-functions@7.20.7: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-async-generator-functions instead.
warning @babel/plugin-proposal-class-properties@7.18.6: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-class-properties instead.
warning @babel/plugin-proposal-export-namespace-from@7.18.9: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-export-namespace-from instead.
warning @babel/plugin-proposal-nullish-coalescing-operator@7.18.6: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-nullish-coalescing-operator instead.
warning @babel/plugin-proposal-object-rest-spread@7.20.7: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-object-rest-spread instead.
warning @babel/plugin-proposal-optional-chaining@7.21.0: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-optional-chaining instead.
warning @babel/polyfill@7.12.1: 🚨 This package has been deprecated in favor of separate inclusion of a polyfill and regenerator-runtime (when needed). See the @babel/polyfill docs (https://babeljs.io/docs/en/babel-polyfill) for more information.
warning @babel/polyfill > core-js@2.6.12: core-js@<3.23.3 is no longer maintained and not recommended for usage due to the number of issues. Because of the V8 engine whims, feature detection in old core-js versions could cause a slowdown up to 100x even if nothing is polyfilled. Some versions have web compatibility issues. Please, upgrade your dependencies to the actual version of core-js.
warning @babel/runtime-corejs2 > core-js@2.6.12: core-js@<3.23.3 is no longer maintained and not recommended for usage due to the number of issues. Because of the V8 engine whims, feature detection in old core-js versions could cause a slowdown up to 100x even if nothing is polyfilled. Some versions have web compatibility issues. Please, upgrade your dependencies to the actual version of core-js.
warning babel-plugin-lodash > glob@7.2.3: Glob versions prior to v9 are no longer supported
warning workspace-aggregator-bb22e1ea-626f-478e-b48b-fa94f4698d09 > eslint@8.57.1: This version is no longer supported. Please see https://eslint.org/version-support for other options.
warning workspace-aggregator-bb22e1ea-626f-478e-b48b-fa94f4698d09 > glob@7.2.3: Glob versions prior to v9 are no longer supported
warning workspace-aggregator-bb22e1ea-626f-478e-b48b-fa94f4698d09 > sinon@9.2.4: 16.1.1
warning workspace-aggregator-bb22e1ea-626f-478e-b48b-fa94f4698d09 > @babel/plugin-proposal-async-generator-functions@7.20.7: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-async-generator-functions instead.
warning workspace-aggregator-bb22e1ea-626f-478e-b48b-fa94f4698d09 > @babel/plugin-proposal-class-properties@7.18.6: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-class-properties instead.
warning workspace-aggregator-bb22e1ea-626f-478e-b48b-fa94f4698d09 > @babel/plugin-proposal-export-namespace-from@7.18.9: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-export-namespace-from instead.
warning workspace-aggregator-bb22e1ea-626f-478e-b48b-fa94f4698d09 > @babel/plugin-proposal-nullish-coalescing-operator@7.18.6: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-nullish-coalescing-operator instead.
warning workspace-aggregator-bb22e1ea-626f-478e-b48b-fa94f4698d09 > @babel/plugin-proposal-object-rest-spread@7.20.7: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-object-rest-spread instead.
warning workspace-aggregator-bb22e1ea-626f-478e-b48b-fa94f4698d09 > @babel/plugin-proposal-optional-chaining@7.21.0: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-optional-chaining instead.
warning workspace-aggregator-bb22e1ea-626f-478e-b48b-fa94f4698d09 > @babel/polyfill@7.12.1: 🚨 This package has been deprecated in favor of separate inclusion of a polyfill and regenerator-runtime (when needed). See the @babel/polyfill docs (https://babeljs.io/docs/en/babel-polyfill) for more information.
[2/4] Fetching packages...
error eslint-plugin-jsdoc@40.3.0: The engine "node" is incompatible with this module. Expected version "^14 || ^16 || ^17 || ^18 || ^19". Got "22.9.0"
error Found incompatible module.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

✨ Finishing Touches
  • 📝 Generate Docstrings

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@bhabalan
Copy link
Contributor Author

bhabalan commented Mar 3, 2025

@coderabbitai review

Copy link

coderabbitai bot commented Mar 3, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🧹 Nitpick comments (8)
packages/@webex/plugin-cc/test/unit/spec/MetricsManager.ts (2)

74-74: Use a more explicit method to reset the singleton between tests.

Directly accessing and modifying a private static property of a class is not a best practice. It would be better to expose a testing-specific method to reset the singleton state or to use proper dependency injection.

-      MetricsManager['instance'] = undefined;
+      // Add a static resetInstance method to MetricsManager class
+      MetricsManager.resetInstance();

Then add this method to the MetricsManager class:

// Add to MetricsManager.ts
public static resetInstance(): void {
  MetricsManager.instance = undefined;
}

3-3:

❓ Verification inconclusive

Avoid importing from implementation files.

Importing types directly from implementation files (like metrics.types.ts) rather than from the package's public API can lead to tight coupling and make your code more brittle to internal changes in the dependency.

-import { EventPayload } from '@webex/internal-plugin-metrics/src/metrics.types';
+import { EventPayload } from '@webex/internal-plugin-metrics';

Let's check if the type is exported from the main package entry point:


🏁 Script executed:

#!/bin/bash
# Check if EventPayload is exported from the main package entry point
rg -A 3 "export.*EventPayload" --type-add "ts:*.{ts,tsx}" --type ts packages/@webex/internal-plugin-metrics/

Length of output: 1534


Update Import to Use the Public API

It looks like the type is originally declared and exported in the internal file (src/metrics.types.ts), but to avoid tight coupling you should import it from the package’s public API. Please ensure that the package’s main entry point (e.g., its index file) properly re‑exports EventPayload so that changing the import won’t break consumers.

  • File: packages/@webex/plugin-cc/test/unit/spec/MetricsManager.ts (Line 3)

  • Action: Update the import statement as follows:

    -import { EventPayload } from '@webex/internal-plugin-metrics/src/metrics.types';
    +import { EventPayload } from '@webex/internal-plugin-metrics';
  • Verification: Confirm that EventPayload is available via the public API. If not, update the main entry point of @webex/internal-plugin-metrics to re-export it.

packages/@webex/plugin-cc/src/MetricsManager.ts (5)

1-1: Avoid importing from implementation files.

Similar to the test file, importing types directly from implementation files rather than from the package's public API can lead to tight coupling.

-import {EventPayload} from '@webex/internal-plugin-metrics/src/metrics.types';
+import {EventPayload} from '@webex/internal-plugin-metrics';

5-5: Make the eventNames type more extensible.

The current implementation limits event names to just three predefined values. This could become a maintenance burden as new events are added. Consider using a more extensible approach.

-type eventNames = 'login' | 'agent-login' | 'agent-logout';
+type CoreEventNames = 'login' | 'agent-login' | 'agent-logout';
+type eventNames = CoreEventNames | string;

Alternatively, if you want to keep the strict typing for event names but make it more maintainable:

enum EventNames {
  Login = 'login',
  AgentLogin = 'agent-login',
  AgentLogout = 'agent-logout'
}
type eventNames = `${EventNames}`;

7-14: Consider using dependency injection instead of a singleton pattern.

The singleton pattern can make testing difficult and creates tight coupling. A more flexible approach would be to use dependency injection, where the MetricsManager is created and passed to components that need it.

While the singleton pattern works, it might be worth considering dependency injection for better testability and flexibility. This would involve:

  1. Removing the static instance property
  2. Making the constructor public
  3. Creating the instance in a higher-level component (e.g., a service locator or composition root)
  4. Passing the instance to components that need it

This approach would also eliminate the need for the getInstance method.


16-26: Add error handling to the metric tracking.

The current implementation silently fails if there's an issue with submitting the metric. Consider adding error logging to capture potential problems.

  public trackBehavioralMetric(eventName: eventNames, data?: EventPayload) {
    if (this.webex?.internal?.newMetrics) {
+     try {
        this.webex.internal.newMetrics.submitBehavioralEvent({
          product: 'wxcc_desktop',
          agent: 'sdk',
          target: eventName,
          verb: 'load',
          payload: data,
        });
+     } catch (error) {
+       console.error('Failed to submit behavioral metric:', error);
+     }
    }
  }

28-35: Add error handling to the operational metric tracking.

Similar to the behavioral metrics, add error handling for operational metrics to prevent silent failures.

  public trackOperationalMetric(eventName: string, payload?: EventPayload) {
    if (this.webex?.internal?.newMetrics) {
+     try {
        this.webex.internal.newMetrics.submitOperationalEvent({
          name: eventName,
          payload,
        });
+     } catch (error) {
+       console.error('Failed to submit operational metric:', error);
+     }
    }
  }
packages/@webex/plugin-cc/src/services/agent/index.ts (1)

62-74: DRY opportunity: Extract common metrics tracking logic.

The metrics tracking code in this error handler is nearly identical to the one in the notifFail section (lines 89-94). Consider extracting this into a helper function to avoid duplication.

+      // Helper function to track login metrics
+      const trackLoginMetrics = (isSuccess: boolean, error?: any) => {
+        metricsManager.trackBehavioralMetric('agent-login', {
+          ...p.data,
+          isSuccess,
+          roles: Array.isArray(p.data.roles) ? p.data.roles.join(',') : '',
+          ...(error && { error: JSON.stringify(error) }),
+        });
+      };
+
       err: /* istanbul ignore next */ (e: any) => {
-        metricsManager.trackBehavioralMetric('agent-login', {
-          ...p.data,
-          isSuccess: false,
-          roles: Array.isArray(p.data.roles) ? p.data.roles.join(',') : '',
-        });
+        trackLoginMetrics(false);

         return new Err.Details('Service.aqm.agent.stationLogin', {
           status: e.response?.status ?? 0,
           type: e.response?.data?.errorType,
           trackingId: e.response?.headers?.trackingid?.split('_')[1],
         });
       },

And then at line 88:

        err(e) {
-          metricsManager.trackBehavioralMetric('agent-login', {
-            ...p.data,
-            isSuccess: false,
-            roles: Array.isArray(p.data.roles) ? p.data.roles.join(',') : '',
-            error: JSON.stringify(e),
-          });
+          trackLoginMetrics(false, e);
        },
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f5a1ab2 and 9a16b46.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (8)
  • packages/@webex/plugin-cc/package.json (1 hunks)
  • packages/@webex/plugin-cc/src/MetricsManager.ts (1 hunks)
  • packages/@webex/plugin-cc/src/cc.ts (4 hunks)
  • packages/@webex/plugin-cc/src/services/agent/index.ts (3 hunks)
  • packages/@webex/plugin-cc/src/services/index.ts (2 hunks)
  • packages/@webex/plugin-cc/src/types.ts (2 hunks)
  • packages/@webex/plugin-cc/test/unit/spec/MetricsManager.ts (1 hunks)
  • packages/@webex/plugin-cc/test/unit/spec/services/agent/index.ts (3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Initialize Project
  • GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (13)
packages/@webex/plugin-cc/src/services/index.ts (2)

1-1: Dependency change aligns with metrics integration

Good addition of the Metrics import from the internal plugin. This aligns with the PR objective of introducing metrics tracking capabilities.


25-25: Clean implementation of metrics dependency injection

Good approach using dependency injection to provide the metrics capability to the routing agent. The singleton pattern with getInstance ensures consistent usage of the same metrics instance throughout the application.

packages/@webex/plugin-cc/src/cc.ts (4)

30-30: Module import properly structured

The import of the MetricsManager is correctly positioned with the other imports.


42-42: Private member correctly defined

Good practice using a private member variable for the metrics manager.


69-69: Proper initialization timing

The initialization of the metrics manager is correctly placed after the webex instance is ready and follows the same singleton pattern used elsewhere in the code.


207-210: Well-structured metrics event implementation

Good implementation of the metrics tracking for agent login:

  1. The timing is correct - after the login response is successfully processed
  2. The event name is clear and descriptive
  3. The data payload contains relevant information without exposing sensitive details

This aligns well with the PR objective of adding behavioral metrics.

packages/@webex/plugin-cc/test/unit/spec/services/agent/index.ts (3)

1-1: Import path correctly structured

The import path for MetricsManager is appropriate for the test file.


14-14: Mock variable declaration looks good

The declaration of the mock MetricsManager follows the same pattern as other mocks in this file.


26-26: Correctly updated function call with new parameter

The call to routingAgent has been correctly updated to include the mock metrics manager, following the implementation change.

packages/@webex/plugin-cc/package.json (1)

28-28:

❓ Verification inconclusive

Dependency updated for metrics functionality

The dependency has been correctly updated from @webex/internal-plugin-mercury to @webex/internal-plugin-metrics to support the new metrics tracking capability.


🏁 Script executed:

#!/bin/bash
# Description: Check if the mercury plugin was used elsewhere in the codebase
# This helps ensure we haven't removed a dependency that's still needed

# Search for imports of the mercury plugin
echo "Checking for imports of @webex/internal-plugin-mercury:"
rg --type-add 'ts:*.{ts,js}' --type ts '@webex/internal-plugin-mercury' --stats

# Check for any potential references to mercury in the code
echo -e "\nChecking for potential references to Mercury functionality:"
rg --type-add 'ts:*.{ts,js}' --type ts -i 'mercury' 'packages/@webex/plugin-cc/' --stats

Length of output: 5065


Metrics dependency updated – please verify lingering Mercury references

The dependency change in packages/@webex/plugin-cc/package.json has been applied correctly by replacing the Mercury dependency with Metrics. However, our search indicates that there are still references to @webex/internal-plugin-mercury and related Mercury terminology in plugin‑cc (e.g., in comments and type definitions in src/cc.ts and src/types.ts). Please review these occurrences and confirm whether they are intentional (for backward compatibility or legacy documentation) or if they should also be updated to align with the new Metrics functionality.

packages/@webex/plugin-cc/src/types.ts (1)

2-2: Good decision to directly import the metric event types.

Using the specific types from @webex/internal-plugin-metrics provides proper type safety and makes the interface definition clearer.

packages/@webex/plugin-cc/src/services/agent/index.ts (2)

8-8: Import looks good.

The import of the MetricsManager is correctly placed and follows the project's import style.


16-16: Updated function signature is necessary for the metrics integration.

The function signature has been appropriately updated to include the MetricsManager parameter, which is required for tracking metrics within the routing agent functionality.

Comment on lines +23 to +24
fakeMetricsManager.trackBehavioralMetric = jest.fn();
fakeMetricsManager.trackOperationalMetric = jest.fn();
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Missing initialization for fakeMetricsManager

The mock functions are being defined on fakeMetricsManager before it's initialized, which would cause runtime errors when the tests are executed.

  fakeAqm = new AqmReqs() as jest.Mocked<AqmReqs>;
  fakeAqm.reqEmpty = jest.fn().mockImplementation((fn) => fn);
  fakeAqm.req = jest.fn().mockImplementation((fn) => fn);
+  fakeMetricsManager = {
+    trackBehavioralMetric: jest.fn(),
+    trackOperationalMetric: jest.fn(),
+  } as unknown as jest.Mocked<MetricsManager>;
-  fakeMetricsManager.trackBehavioralMetric = jest.fn();
-  fakeMetricsManager.trackOperationalMetric = jest.fn();
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fakeMetricsManager.trackBehavioralMetric = jest.fn();
fakeMetricsManager.trackOperationalMetric = jest.fn();
fakeAqm = new AqmReqs() as jest.Mocked<AqmReqs>;
fakeAqm.reqEmpty = jest.fn().mockImplementation((fn) => fn);
fakeAqm.req = jest.fn().mockImplementation((fn) => fn);
fakeMetricsManager = {
trackBehavioralMetric: jest.fn(),
trackOperationalMetric: jest.fn(),
} as unknown as jest.Mocked<MetricsManager>;

Comment on lines +118 to 121
newMetrics: {
submitBehavioralEvent: SubmitBehavioralEvent;
submitOperationalEvent: SubmitOperationalEvent;
};
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Consider using a more permanent property name instead of "newMetrics".

The property name "newMetrics" suggests a transitional approach. Since this appears to be a permanent refactoring of how metrics are handled, consider using a more semantic name like "metrics" or "metricsService" for long-term maintainability.

-  newMetrics: {
+  metrics: {
     submitBehavioralEvent: SubmitBehavioralEvent;
     submitOperationalEvent: SubmitOperationalEvent;
   };

Let's check if there are other occurrences of "newMetrics" in the codebase to understand the scope of this change:


🏁 Script executed:

#!/bin/bash
# Find all occurrences of "newMetrics" to evaluate the impact of renaming
rg "newMetrics" --type-add "ts:*.{ts,tsx}" --type ts

Length of output: 32253


Action: Rename "newMetrics" to a permanent, semantic property name across the codebase.

The identifier "newMetrics" is used extensively—not only in the type definitions in packages/@webex/plugin-cc/src/types.ts but also across modules in MetricsManager, various tests, and even in plugins like internal-plugin-metrics and plugin-meetings. Since this represents the permanent approach for handling metrics, it would improve long-term maintainability to adopt a more descriptive name such as "metrics" or "metricsService".

Recommendations:

  • Type Definition: Update the property name in packages/@webex/plugin-cc/src/types.ts.
  • Implementation Files: Change all references in files like packages/@webex/plugin-cc/src/MetricsManager.ts accordingly.
  • Tests and Other Modules: Ensure that any unit tests or other modules (e.g., files under internal-plugin-metrics and plugin-meetings) that refer to "newMetrics" are also updated to maintain consistency.

Diff Suggestion:

-  newMetrics: {
+  metrics: {
     submitBehavioralEvent: SubmitBehavioralEvent;
     submitOperationalEvent: SubmitOperationalEvent;
   };

Please make sure that if you decide to rename "newMetrics", all affected files are updated to prevent any potential reference errors.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
newMetrics: {
submitBehavioralEvent: SubmitBehavioralEvent;
submitOperationalEvent: SubmitOperationalEvent;
};
metrics: {
submitBehavioralEvent: SubmitBehavioralEvent;
submitOperationalEvent: SubmitOperationalEvent;
};

Comment on lines +51 to +63
// describe('trackOperationalMetric', () => {
// it('should submit an operational event when newMetrics is available', () => {
// metricsManager.trackOperationalMetric();

// expect(webex.internal.newMetrics.submitOperationalEvent).toHaveBeenCalledWith({
// name: 'ccsdk.event',
// payload: {
// action: 'load',
// },
// });
// });

// });
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Uncomment and implement the operational metrics test.

The operational metrics testing is commented out, which means this functionality isn't being verified. As operational metrics are a key part of the new feature, it's important to test them properly.

-  // describe('trackOperationalMetric', () => {
-  //   it('should submit an operational event when newMetrics is available', () => {
-  //     metricsManager.trackOperationalMetric();
-
-  //     expect(webex.internal.newMetrics.submitOperationalEvent).toHaveBeenCalledWith({
-  //       name: 'ccsdk.event',
-  //       payload: {
-  //         action: 'load',
-  //       },
-  //     });
-  //   });
-
-  // });
+  describe('trackOperationalMetric', () => {
+    it('should submit an operational event when newMetrics is available', () => {
+      const eventName = 'ccsdk.event';
+      const payload = { action: 'load' };
+      
+      metricsManager.trackOperationalMetric(eventName, payload);
+
+      expect(webex.internal.newMetrics.submitOperationalEvent).toHaveBeenCalledWith({
+        name: eventName,
+        payload,
+      });
+    });
+    
+    it('should not submit an operational event when newMetrics is not available', () => {
+      webex.internal.newMetrics = undefined as any;
+      
+      metricsManager.trackOperationalMetric('ccsdk.event');
+      
+      // Test passes if there's no error
+      expect(true).toBe(true);
+    });
+  });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// describe('trackOperationalMetric', () => {
// it('should submit an operational event when newMetrics is available', () => {
// metricsManager.trackOperationalMetric();
// expect(webex.internal.newMetrics.submitOperationalEvent).toHaveBeenCalledWith({
// name: 'ccsdk.event',
// payload: {
// action: 'load',
// },
// });
// });
// });
describe('trackOperationalMetric', () => {
it('should submit an operational event when newMetrics is available', () => {
const eventName = 'ccsdk.event';
const payload = { action: 'load' };
metricsManager.trackOperationalMetric(eventName, payload);
expect(webex.internal.newMetrics.submitOperationalEvent).toHaveBeenCalledWith({
name: eventName,
payload,
});
});
it('should not submit an operational event when newMetrics is not available', () => {
webex.internal.newMetrics = undefined as any;
metricsManager.trackOperationalMetric('ccsdk.event');
// Test passes if there's no error
expect(true).toBe(true);
});
});

Comment on lines +42 to +48
// it('should not submit a behavioral event when newMetrics is not available', () => {
// webex.internal.newMetrics = undefined;

// metricsManager.trackBehavioralMetric('login');

// expect(webex.internal.newMetrics?.submitBehavioralEvent).not.toHaveBeenCalled();
// });
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Uncomment and implement the negative test case.

Commented-out tests reduce the coverage and can lead to gaps in functionality testing. The case for when newMetrics is not available is important, as it tests a defensive coding pattern in the implementation.

-    // it('should not submit a behavioral event when newMetrics is not available', () => {
-    //   webex.internal.newMetrics = undefined;
-
-    //   metricsManager.trackBehavioralMetric('login');
-
-    //   expect(webex.internal.newMetrics?.submitBehavioralEvent).not.toHaveBeenCalled();
-    // });
+    it('should not submit a behavioral event when newMetrics is not available', () => {
+      webex.internal.newMetrics = undefined as any;
+
+      metricsManager.trackBehavioralMetric('login');
+
+      // Test passes if there's no error
+      expect(true).toBe(true);
+    });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// it('should not submit a behavioral event when newMetrics is not available', () => {
// webex.internal.newMetrics = undefined;
// metricsManager.trackBehavioralMetric('login');
// expect(webex.internal.newMetrics?.submitBehavioralEvent).not.toHaveBeenCalled();
// });
it('should not submit a behavioral event when newMetrics is not available', () => {
webex.internal.newMetrics = undefined as any;
metricsManager.trackBehavioralMetric('login');
// Test passes if there's no error
expect(true).toBe(true);
});

Comment on lines +37 to +44
// Make the class a singleton
public static getInstance(options: {webex: WebexSDK}): MetricsManager {
if (!MetricsManager.instance) {
MetricsManager.instance = new MetricsManager(options);
}

return MetricsManager.instance;
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add safety checks in getInstance to prevent invalid initialization.

The getInstance method doesn't validate that options.webex is provided. This could lead to runtime errors if the method is called without the required parameters.

  public static getInstance(options: {webex: WebexSDK}): MetricsManager {
+   if (!options || !options.webex) {
+     throw new Error('WebexSDK instance is required to create a MetricsManager');
+   }
+   
    if (!MetricsManager.instance) {
      MetricsManager.instance = new MetricsManager(options);
    }

    return MetricsManager.instance;
  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Make the class a singleton
public static getInstance(options: {webex: WebexSDK}): MetricsManager {
if (!MetricsManager.instance) {
MetricsManager.instance = new MetricsManager(options);
}
return MetricsManager.instance;
}
// Make the class a singleton
public static getInstance(options: {webex: WebexSDK}): MetricsManager {
if (!options || !options.webex) {
throw new Error('WebexSDK instance is required to create a MetricsManager');
}
if (!MetricsManager.instance) {
MetricsManager.instance = new MetricsManager(options);
}
return MetricsManager.instance;
}

Comment on lines +88 to +95
err(e) {
metricsManager.trackBehavioralMetric('agent-login', {
...p.data,
isSuccess: false,
roles: Array.isArray(p.data.roles) ? p.data.roles.join(',') : '',
error: JSON.stringify(e),
});
},
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Track success metrics as well as failures.

Currently, metrics are tracked only in error scenarios. For completeness and to get a full picture of the system's performance, it would be valuable to track successful operations as well.

Add a success tracking implementation in the notifSuccess handler:

      notifSuccess: {
        bind: {
          type: CC_EVENTS.AGENT_STATION_LOGIN,
          data: {type: CC_EVENTS.AGENT_STATION_LOGIN_SUCCESS},
        },
        msg: {} as Agent.StationLoginSuccess,
+       success() {
+         metricsManager.trackBehavioralMetric('agent-login', {
+           ...p.data,
+           isSuccess: true,
+           roles: Array.isArray(p.data.roles) ? p.data.roles.join(',') : ''
+         });
+       },
      },
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
err(e) {
metricsManager.trackBehavioralMetric('agent-login', {
...p.data,
isSuccess: false,
roles: Array.isArray(p.data.roles) ? p.data.roles.join(',') : '',
error: JSON.stringify(e),
});
},
err(e) {
metricsManager.trackBehavioralMetric('agent-login', {
...p.data,
isSuccess: false,
roles: Array.isArray(p.data.roles) ? p.data.roles.join(',') : '',
error: JSON.stringify(e),
});
},
notifSuccess: {
bind: {
type: CC_EVENTS.AGENT_STATION_LOGIN,
data: { type: CC_EVENTS.AGENT_STATION_LOGIN_SUCCESS },
},
msg: {} as Agent.StationLoginSuccess,
success() {
metricsManager.trackBehavioralMetric('agent-login', {
...p.data,
isSuccess: true,
roles: Array.isArray(p.data.roles) ? p.data.roles.join(',') : ''
});
},
},

Copy link

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

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

@@ -25,7 +25,7 @@
"dependencies": {
"@types/platform": "1.3.4",
"@webex/calling": "workspace:*",
"@webex/internal-plugin-mercury": "workspace:*",
Copy link
Collaborator

Choose a reason for hiding this comment

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

why removing mercury here ?

public trackBehavioralMetric(eventName: eventNames, data?: EventPayload) {
if (this.webex?.internal?.newMetrics) {
this.webex.internal.newMetrics.submitBehavioralEvent({
product: 'wxcc_desktop',
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you need a wrapper , can you check the web client if they do it based on config for adding default values ?

@@ -201,6 +204,11 @@ export default class ContactCenter extends WebexPlugin implements IContactCenter

await loginResponse;

this.metricsManager.trackBehavioralMetric('agent-login', {
Copy link
Collaborator

Choose a reason for hiding this comment

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

all constnt to config file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
validated If the pull request is validated for automation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants