feat(internal-plugin-metrics): allow caller to specify metadata of business metric events #4000
Conversation
WalkthroughThe changes in this pull request focus on enhancing the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) 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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/@webex/internal-plugin-metrics/src/business-metrics.ts (2)
65-71: Fix typo in documentationThe JSDoc comment contains a typo in the word "buisness".
- * Submit a buisness metric to our metrics endpoint + * Submit a business metric to our metrics endpoint
72-89: Consider refactoring to reduce code duplicationThe event construction logic is very similar across all submit methods. Consider extracting the common event construction logic into a shared private method.
Example refactor:
private createBaseEvent(name: string, payload: EventPayload, appType: string, options?: {indexHint?: string}) { return { type: ['business'], eventPayload: { key: name, client_timestamp: new Date().toISOString(), appType, ...(options?.indexHint && {indexHint: options.indexHint}), value: { ...this.getContext(), ...this.getBrowserDetails(), ...payload, }, }, }; } private submitWxccDesktopEvent({name, payload}: {name: string; payload: EventPayload}) { const event = this.createBaseEvent(name, payload, 'WxCC Desktop', {indexHint: 'wxccdesktop'}); return this.submitEvent({ kind: 'business-events:business_metrics_wxcc_desktop -> ', name, event, }); }packages/@webex/internal-plugin-metrics/src/metrics.types.ts (1)
172-177: LGTM! Consider adding documentation.The addition of
'business_metrics_wxcc_desktop'to theTabletype is well-structured and aligns with the FedRAMP compliance requirements. The change is non-breaking and follows the existing naming patterns.Consider adding a JSDoc comment above the
Tabletype to document the purpose of each table, particularly the newbusiness_metrics_wxcc_desktoptable. This would improve maintainability and help other developers understand when to use each table type.+/** + * Defines the available metric table destinations + * @property business_metrics_wxcc_desktop - Table for FedRAMP compliance metrics from wxcc desktop client + * ... document other table types ... + */ export type Table = | 'wbxapp_callend_metrics' | 'business_metrics' | 'business_ucf' | 'business_metrics_wxcc_desktop' | 'default';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/@webex/internal-plugin-metrics/src/business-metrics.ts(2 hunks)packages/@webex/internal-plugin-metrics/src/metrics.types.ts(1 hunks)packages/@webex/internal-plugin-metrics/test/unit/spec/business/business-metrics.ts(1 hunks)
🧰 Additional context used
🪛 Biome
packages/@webex/internal-plugin-metrics/test/unit/spec/business/business-metrics.ts
[error] 208-208: This property value named locale is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named locale
(lint/suspicious/noDuplicateObjectKeys)
🔇 Additional comments (5)
packages/@webex/internal-plugin-metrics/src/business-metrics.ts (2)
141-142: LGTM!
The switch case addition is clean and follows the established pattern.
Line range hint 65-142: Verify test coverage for the new functionality
The PR objectives mention that unit tests were added, but they're not visible in the provided files. Please ensure that:
- Test cases cover the new
submitWxccDesktopEventmethod - Test cases verify the correct event structure including the
indexHintandappTypefields - Test cases validate the new table option in
submitBusinessEvent
✅ Verification successful
Let me check the test file content to verify the test coverage.
Test coverage is complete and accurate
The test file includes comprehensive coverage for the new functionality:
- Test case verifies
submitWxccDesktopEventthrough thebusiness_metrics_wxcc_desktoptable option - Validates correct event structure including
indexHint: 'wxccdesktop'andappType: 'WxCC Desktop' - Asserts the proper payload structure and timestamp handling
- Confirms the integration with the table routing in
submitBusinessEvent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related test files and their coverage of the new functionality
fd -e test.ts -e spec.ts . | xargs rg -l "business_metrics_wxcc_desktop"
# Look for existing test patterns that should be followed
rg -A 5 "describe.*BusinessMetrics"
Length of output: 754
Script:
#!/bin/bash
# Check the content of the test file
cat "packages/@webex/internal-plugin-metrics/test/unit/spec/business/business-metrics.ts"
Length of output: 8712
packages/@webex/internal-plugin-metrics/src/metrics.types.ts (1)
172-177: Verify handling of the new table type.
Let's ensure the new table type is properly handled throughout the codebase.
✅ Verification successful
Let me generate another script to check for switch statements handling the table type more specifically:
The new table type is properly handled throughout the codebase
The verification shows that:
- The new table type is properly handled in the switch statement with a dedicated case that maps to
submitWxccDesktopEvent - The type is properly defined in metrics.types.ts
- The implementation includes proper documentation for the new table type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify proper handling of the new table type
# Check for switch/case statements that handle table types
echo "Checking for switch statements handling table types..."
rg -A 5 "switch.*\(.*[tT]able.*\)"
# Check for hardcoded table type strings
echo "Checking for hardcoded table type strings..."
rg -i "business_metrics_wxcc_desktop"
# Check for type imports
echo "Checking for Table type imports..."
rg "import.*\{.*Table.*\}.*from.*metrics\.types"
Length of output: 132172
Script:
#!/bin/bash
# Check for switch statements handling table types in business-metrics.ts
echo "Checking switch statement handling of table types..."
rg -A 10 "switch.*\(.*table.*\)" packages/@webex/internal-plugin-metrics/src/business-metrics.ts
# Check for any other references to the table type
echo "Checking for other table type references..."
rg -A 2 "business_metrics_wxcc_desktop" packages/@webex/internal-plugin-metrics/src/
Length of output: 1793
packages/@webex/internal-plugin-metrics/test/unit/spec/business/business-metrics.ts (2)
185-219: LGTM! Test structure follows established patterns.
The new test case is well-organized and follows the existing test structure, maintaining consistency with other table-specific tests.
🧰 Tools
🪛 Biome
[error] 208-208: This property value named locale is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named locale
(lint/suspicious/noDuplicateObjectKeys)
185-219: Verify FedRAMP compliance requirements coverage.
While the test covers the basic functionality, please verify if there are any additional FedRAMP-specific requirements that need to be validated in the test case, such as:
- Required fields for compliance
- Data format validations
- Security-related assertions
Let me help verify the FedRAMP requirements:
🧰 Tools
🪛 Biome
[error] 208-208: This property value named locale is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named locale
(lint/suspicious/noDuplicateObjectKeys)
packages/@webex/internal-plugin-metrics/src/business-metrics.ts
Outdated
Show resolved
Hide resolved
packages/@webex/internal-plugin-metrics/test/unit/spec/business/business-metrics.ts
Outdated
Show resolved
Hide resolved
packages/@webex/internal-plugin-metrics/src/business-metrics.ts
Outdated
Show resolved
Hide resolved
lialang-cisco
force-pushed
the
wxccdesktop
branch
from
ea3c708
to
7a919a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
packages/@webex/internal-plugin-metrics/src/business-metrics.ts (2)
11-27: Improve type safety and documentation ofbuildEventmethod.The method implementation looks good, but could benefit from stronger typing and better documentation:
/** + * @private * Build the metric event to submit. * @param {string} name of the metric * @param {EventPayload} payload user payload of the metric * @param {EventPayload} metadata to include outside of eventPayload.value - * @returns {object} + * @returns {MetricEvent} The constructed metric event */ private buildEvent({name, payload, metadata}: { name: string; - payload: object; - metadata: object; + payload: EventPayload; + metadata: EventPayload; }): MetricEvent {Consider adding a
MetricEventinterface to define the return type structure.
Update the
Tabletype to include the new FedRAMP tableThe
Tabletype inmetrics.types.tsneeds to be updated to include the newbusiness_metrics_wxcc_desktoptable for FedRAMP compliance. Also, there's a typo in the JSDoc comment where "buisness" should be "business".
- In
packages/@webex/internal-plugin-metrics/src/metrics.types.ts:export type Table = 'wbxapp_callend_metrics' | 'business_metrics' | 'business_ucf' | 'business_metrics_wxcc_desktop' | 'default';- In
packages/@webex/internal-plugin-metrics/src/business-metrics.ts:/** * Submit a business metric to our metrics endpoint. */🔗 Analysis chain
Line range hint
28-49: Fix documentation issues and update Table type.
- There's a typo in "buisness" in the JSDoc.
- The
Tabletype needs to be updated to include the newbusiness_metrics_wxcc_desktoptable mentioned in the PR objectives./** - * Submit a buisness metric to our metrics endpoint. + * Submit a business metric to our metrics endpoint. */Let's verify if the
Tabletype has been updated:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if Table type includes the new table rg -A 10 "export type Table =" packages/@webex/internal-plugin-metrics/src/metrics.types.tsLength of output: 583
🧰 Tools
🪛 Biome (1.9.4)
[error] 93-93: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 94-94: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
packages/@webex/internal-plugin-metrics/test/unit/spec/new-metrics.ts (1)
96-103: Enhance test coverage for metadata parameterWhile the basic metadata functionality is tested, consider adding the following test cases for more comprehensive coverage:
- Test with actual FedRAMP-related metadata fields
- Test with missing/undefined metadata (optional parameter)
- Test with the specific
business_metrics_wxcc_desktoptable mentioned in PR objectivesExample test case structure:
it('handles business_metrics_wxcc_desktop table with FedRAMP metadata', () => { webex.internal.newMetrics.submitBusinessEvent({ name: 'fedRampMetric', payload: {}, table: 'business_metrics_wxcc_desktop', metadata: { complianceFramework: 'FedRAMP', securityLevel: 'moderate', // Add other relevant FedRAMP fields }, }); // Assert the call }); it('handles missing metadata parameter', () => { webex.internal.newMetrics.submitBusinessEvent({ name: 'metricWithoutMeta', payload: {}, table: 'business_metrics_wxcc_desktop' }); // Assert the call });packages/@webex/internal-plugin-metrics/src/new-metrics.ts (2)
210-216: Add JSDoc documentation for the metadata parameterThe implementation looks good, but please add documentation for the new
metadataparameter to maintain consistency with the existing JSDoc comments.Apply this diff to add the documentation:
/** * Buisness event * @param args + * @param {EventPayload} [args.metadata] - Optional metadata for the business event */
Line range hint
210-228: Consider adding metadata validationThe implementation correctly handles the new metadata parameter, but consider adding validation to ensure the metadata meets any FedRAMP compliance requirements before passing it to the business metrics table.
Example validation approach:
submitBusinessEvent({ name, payload, table, metadata, }: { name: string; payload: EventPayload; table?: Table; metadata?: EventPayload; }) { if (!this.isReady) { // @ts-ignore this.webex.logger.log( `NewMetrics: @submitBusinessEvent. Attempted to submit before webex.ready: ${name}` ); return Promise.resolve(); } this.lazyBuildBusinessMetrics(); + // Validate metadata if present + if (metadata) { + // Add validation logic specific to FedRAMP requirements + // Example: Check for required fields, data types, or sensitive information + if (!this.isValidMetadata(metadata)) { + return Promise.reject(new Error('Invalid metadata format for FedRAMP compliance')); + } + } + return this.businessMetrics.submitBusinessEvent({name, payload, table, metadata}); } + + /** + * Validates metadata for FedRAMP compliance + * @private + */ + private isValidMetadata(metadata: EventPayload): boolean { + // Add validation rules based on FedRAMP requirements + return true; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/@webex/internal-plugin-metrics/src/business-metrics.ts(2 hunks)packages/@webex/internal-plugin-metrics/src/new-metrics.ts(2 hunks)packages/@webex/internal-plugin-metrics/test/unit/spec/business/business-metrics.ts(1 hunks)packages/@webex/internal-plugin-metrics/test/unit/spec/new-metrics.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/internal-plugin-metrics/test/unit/spec/business/business-metrics.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/@webex/internal-plugin-metrics/src/business-metrics.ts
[error] 93-93: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 94-94: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
🔇 Additional comments (3)
packages/@webex/internal-plugin-metrics/src/business-metrics.ts (2)
60-91:
Fix typo in event kind parameter.
There's a typo in the event kind parameter:
- kind: 'buisness-events:wbxapp_callend_metrics -> ',
+ kind: 'business-events:wbxapp_callend_metrics -> ',- kind: 'buisness-events:business_metrics -> ',
+ kind: 'business-events:business_metrics -> ',53-58: Review default appType for wxcc_desktop metrics.
The default appType is set to 'Web Client', which might not be appropriate for metrics being sent to the business_metrics_wxcc_desktop table. Consider making this configurable or setting it based on the target table.
Let's check for other appType usages:
packages/@webex/internal-plugin-metrics/src/new-metrics.ts (1)
228-228: Verify metadata handling in BusinessMetrics class
Let's verify how the metadata is handled in the BusinessMetrics class implementation.
✅ Verification successful
Metadata is properly handled in BusinessMetrics implementation
The BusinessMetrics class correctly implements metadata handling:
- The
submitBusinessEventmethod accepts metadata as an optional parameter - Metadata is properly initialized to an empty object if not provided
- The
buildEventmethod correctly incorporates metadata into the eventPayload structure - Different table cases (
wbxapp_callend_metrics,business_metrics,business_ucf/default) all properly handle the metadata parameter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of metadata handling in BusinessMetrics class
# Look for the submitBusinessEvent method in BusinessMetrics class
ast-grep --pattern 'class BusinessMetrics {
$$$
submitBusinessEvent($_) {
$$$
}
$$$
}'
# Search for any existing metadata handling
rg -l "metadata.*EventPayload" --type ts
Length of output: 266
Script:
#!/bin/bash
# Let's check the implementation of submitBusinessEvent in business-metrics.ts
rg -A 10 "submitBusinessEvent" packages/@webex/internal-plugin-metrics/src/business-metrics.ts
# Also check the type definition for metadata
rg "interface.*EventPayload" -A 5 packages/@webex/internal-plugin-metrics/src/business-metrics.ts
Length of output: 395
Script:
#!/bin/bash
# Let's get more context around the submitBusinessEvent implementation
rg -B 5 -A 15 "submitBusinessEvent" packages/@webex/internal-plugin-metrics/src/business-metrics.ts
# Also check the full file content to understand the metadata handling
cat packages/@webex/internal-plugin-metrics/src/business-metrics.ts
Length of output: 4276
lialang-cisco
changed the title
| assert.equal(requestCalls.length, 1) | ||
| assert.deepEqual(requestCalls[0], { | ||
| eventPayload: { | ||
| key: 'foobar', | ||
| asdf: 'hjkl', |
There was a problem hiding this comment.
So, if the given metadata field already exists in the request, then this input will override it?
There was a problem hiding this comment.
Yep, that's right
| @@ -173,7 +174,6 @@ describe('internal-plugin-metrics', () => { | |||
| os: getOSNameInternal(), | |||
| app: {version: 'webex-version'}, | |||
| device: {id: 'deviceId'}, | |||
| locale: 'language', | |||
There was a problem hiding this comment.
duplicate, locale is already included a few lines above
COMPLETES #CX-17090
This pull request addresses CX-17090
by making the following changes, we allow consumers of the SDK to specify the metadata of business metric events that they send to Pinot.
This can be used to set the
appTypeandindexHintfor example.Change Type
The following scenarios were tested
Sent a metric event from the wxcc_desktop client using this new code path, and observed it in Pinot in the desired table. Added unit test.
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.
Summary by CodeRabbit
New Features
metadataparameter for improved flexibility in thesubmitBusinessEventmethod.Bug Fixes
metadataparameter, enhancing error handling and logging mechanisms.Tests
NewMetricsclass to validate functionality and error handling scenarios related to event submissions.