-
Notifications
You must be signed in to change notification settings - Fork 69
fix(cc-ui-logging): @webex/ui-logging package #491
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
Changes from all commits
136e550
92d37d1
032fd09
e800f38
c4982b2
594e859
a5bfab9
979f3d0
cab57af
3c85248
79c6fe3
40ded98
b73d233
ebdbca5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ import { | |
| CUSTOMER_NAME, | ||
| RONA, | ||
| } from '../constants'; | ||
| import {withMetrics} from '@webex/cc-ui-logging'; | ||
|
|
||
| const CallControlCADComponent: React.FC<CallControlComponentProps> = (props) => { | ||
| const { | ||
|
|
@@ -235,4 +236,5 @@ const CallControlCADComponent: React.FC<CallControlComponentProps> = (props) => | |
| ); | ||
| }; | ||
|
|
||
| export default CallControlCADComponent; | ||
| const CallControlCADComponentWithMetrics = withMetrics(CallControlCADComponent, 'CallControlCAD'); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CallControlComponent is already wrapper in withMetrics, and we are again wrapping the component here with withMetrics. See if the logging is happening twice? If it is then it is wrong. we only need it once |
||
| export default CallControlCADComponentWithMetrics; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -95,4 +95,4 @@ | |
| "^.+\\.(css|less|scss)$": "babel-jest" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,4 +65,4 @@ | |
| "react": ">=18.3.1", | ||
| "react-dom": ">=18.3.1" | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,4 +59,4 @@ | |
| "webpack-cli": "5.1.4", | ||
| "webpack-merge": "6.0.1" | ||
| } | ||
| } | ||
| } | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. revert |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,4 +64,4 @@ | |
| "react": ">=18.3.1", | ||
| "react-dom": ">=18.3.1" | ||
| } | ||
| } | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why were these changes introduced? Can we revert them ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are linting changes, this needs to be fixed in the tooling. We already have a task for this |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,6 @@ import {render, screen, cleanup} from '@testing-library/react'; | |
| import '@testing-library/jest-dom'; | ||
| import {TaskList} from '../../src/TaskList'; | ||
| import * as helper from '../../src/helper'; | ||
| import * as components from '@webex/cc-components'; | ||
| import store from '@webex/cc-store'; | ||
|
|
||
| // Mock `@webex/cc-store`. | ||
|
|
@@ -31,7 +30,6 @@ jest.mock('@webex/cc-store', () => ({ | |
| })); | ||
|
|
||
| describe('TaskList Component', () => { | ||
| const taskListComponentSpy = jest.spyOn(components, 'TaskListComponent'); | ||
| const helperSpy = jest.spyOn(helper, 'useTaskList'); | ||
| afterEach(cleanup); | ||
|
|
||
|
|
@@ -42,20 +40,6 @@ describe('TaskList Component', () => { | |
| const taskListPresentational = screen.getByTestId('task-list'); | ||
| expect(taskListPresentational).toBeInTheDocument(); | ||
|
|
||
| // Verify that `TaskListPresentational` is called with the correct props. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this removed ? |
||
| expect(taskListComponentSpy).toHaveBeenCalledWith( | ||
| { | ||
| currentTask: undefined, | ||
| isBrowser: true, | ||
| logger: store.logger, | ||
| taskList: taskListMock, | ||
| acceptTask: expect.any(Function), | ||
| declineTask: expect.any(Function), | ||
| onTaskSelect: expect.any(Function), | ||
| }, | ||
| {} | ||
| ); | ||
|
|
||
| // Verify that `useTaskList` is called with the correct arguments. | ||
| expect(helperSpy).toHaveBeenCalledWith({ | ||
| cc: store.cc, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| const jestConfig = require('../../../jest.config.js'); | ||
|
|
||
| jestConfig.rootDir = '../../../'; | ||
| jestConfig.testMatch = ['**/ui-logging/tests/**/*.ts', '**/ui-logging/tests/**/*.tsx']; | ||
|
|
||
| module.exports = jestConfig; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| { | ||
| "name": "@webex/cc-ui-logging", | ||
| "version": "1.0.0", | ||
| "description": "UI metrics tracking for Webex widgets", | ||
| "main": "dist/index.js", | ||
| "types": "dist/index.d.ts", | ||
| "scripts": { | ||
| "build:src": "webpack --mode=development", | ||
| "clean": "rm -rf dist", | ||
| "clean:dist": "rm -rf dist", | ||
| "test:unit": "jest --coverage", | ||
| "test:styles": "echo 'No styles to test'" | ||
| }, | ||
| "dependencies": { | ||
| "@webex/cc-store": "workspace:*" | ||
| }, | ||
| "devDependencies": { | ||
| "@testing-library/dom": "10.4.0", | ||
| "@testing-library/jest-dom": "6.6.2", | ||
| "@testing-library/react": "16.0.1", | ||
| "@types/jest": "29.5.14", | ||
| "@types/react-test-renderer": "18", | ||
| "@webex/test-fixtures": "workspace:*", | ||
| "babel-jest": "29.7.0", | ||
| "jest": "29.7.0", | ||
| "jest-environment-jsdom": "29.7.0", | ||
| "typescript": "^5.6.3", | ||
| "uuid": "^9.0.0", | ||
| "webpack": "^5.96.1", | ||
| "webpack-cli": "^5.1.4" | ||
| }, | ||
| "peerDependencies": { | ||
| "react": ">=18.3.1", | ||
| "react-dom": ">=18.3.1" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import withMetrics from './withMetrics'; | ||
| import {WidgetMetrics} from './metricsLogger'; | ||
|
|
||
| export {withMetrics}; | ||
| export type {WidgetMetrics}; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| import store from '@webex/cc-store'; | ||
|
|
||
| export type WidgetMetrics = { | ||
| widgetName: string; | ||
| event: 'WIDGET_MOUNTED' | 'ERROR' | 'WIDGET_UNMOUNTED' | 'PROPS_UPDATED'; | ||
| props?: Record<string, any>; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there no way to avoid using any for props and additionalContext ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, we dont for now. props can be from any widget. If I use the actual types from the widgets it would end up creating a circular dependency. 1 thing we can do is move all the types to store but that would go against out design |
||
| timestamp: number; | ||
| additionalContext?: Record<string, any>; | ||
| }; | ||
|
|
||
| /** | ||
| * Logs UI metrics for contact center widgets. | ||
| * | ||
| * This function logs widget lifecycle events and errors to help monitor | ||
| * widget performance and user interactions. If no logger is available, | ||
| * it will emit a warning and skip logging. | ||
| * | ||
| * @param metric - The metrics data to be logged | ||
| * @param metric.widgetName - Name of the widget generating the metric | ||
| * @param metric.event - Type of event being logged | ||
| * @param metric.props - Optional properties associated with the widget | ||
| * @param metric.timestamp - Unix timestamp when the event occurred | ||
| * @param metric.additionalContext - Optional additional context data | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * logMetrics({ | ||
| * widgetName: 'CallControl', | ||
| * event: 'WIDGET_MOUNTED', | ||
| * props: { callId: '123' }, | ||
| * timestamp: Date.now(), | ||
| * additionalContext: { userId: 'user123' } | ||
| * }); | ||
| * ``` | ||
| */ | ||
| export const logMetrics = (metric: WidgetMetrics) => { | ||
| if (!store.logger) { | ||
| console.warn('CC-Widgets: UI Metrics: No logger found'); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we avoid console.warn ? We can use logger.warn
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this scenario is for when logger is not present. so we are just logging a warning |
||
| return; | ||
| } | ||
| store.logger.log(`CC-Widgets: UI Metrics: ${JSON.stringify(metric, null, 2)}`, { | ||
| module: 'metricsLogger.tsx', | ||
| method: 'logMetrics', | ||
| }); | ||
| }; | ||
|
|
||
| /** | ||
| * Determines if props have changed between two objects using shallow comparison. | ||
| * | ||
| * This function performs a shallow comparison between two objects to detect changes. | ||
| * It compares object keys and primitive values, but does not recursively compare | ||
| * nested objects. This is useful for determining when to log metrics based on prop changes. | ||
| * | ||
| * @param prev - The previous props object | ||
| * @param next - The next props object to compare against | ||
| * @returns `true` if the props have changed, `false` otherwise | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * const oldProps = { name: 'John', age: 30 }; | ||
| * const newProps = { name: 'John', age: 31 }; | ||
| * | ||
| * if (havePropsChanged(oldProps, newProps)) { | ||
| * // Props have changed, log metrics | ||
| * logMetrics({ | ||
| * widgetName: 'UserProfile', | ||
| * event: 'WIDGET_MOUNTED', | ||
| * props: newProps, | ||
| * timestamp: Date.now() | ||
| * }); | ||
| * } * ``` | ||
| * | ||
| * @remarks | ||
| * The function is important as we dont sanitize our props right now. | ||
| * Once we start sanitizing we can do a deep comparison. This is used to only re-render | ||
| * the HOC if the props have changed. | ||
| */ | ||
| export function havePropsChanged(prev: any, next: any): boolean { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment here about any
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above |
||
| if (prev === next) return false; | ||
|
|
||
| // Do shallow comparison | ||
| if (typeof prev !== typeof next) return true; | ||
| if (!prev || !next) return prev !== next; | ||
|
|
||
| const prevKeys = Object.keys(prev); | ||
| const nextKeys = Object.keys(next); | ||
|
|
||
| if (prevKeys.length !== nextKeys.length) return true; | ||
|
|
||
| // Check if any primitive values changed | ||
| for (const key of prevKeys) { | ||
| const prevVal = prev[key]; | ||
| const nextVal = next[key]; | ||
|
|
||
| if (prevVal === nextVal) continue; | ||
| if (typeof prevVal !== 'object' || prevVal === null) return true; | ||
| if (typeof nextVal !== 'object' || nextVal === null) return true; | ||
| } | ||
|
|
||
| // All shallow comparisons passed, consider props unchanged | ||
| return false; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where do we log metrics for prop updated case or is that what that JIra is added in comment ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The JIRA has been added @ packages/contact-center/ui-logging/src/withMetrics.tsx |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we removing this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its not removed, just added another dependency and this line it added a comma that why it is showing like this