feat: add device permission diagnostics metrics - #5155
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8285190d3b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!projectedPermission) { | ||
| return payload; |
There was a problem hiding this comment.
When a registered provider returns undefined (or no projected resources) for a terminal event such as client.call.leave, this early return skips the final-event cleanup just below it. If the same permission scope is reused later, the stale lastReportedPrivacyAndSecurityPermission entry makes the next call look unchanged and suppresses the initial permission metrics, and it also leaves one map entry behind for each affected completed meeting. Move the isFinalEvent delete before this return path as well.
Useful? React with 👍 / 👎.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
chrisadubois
left a comment
chrisadubois
left a comment
There was a problem hiding this comment.
overall looks good but just because I raised this design question as a thought, I asked an agent to make a draft PR that would demonstrate some of those changes, take them or leave them. PR is approved as is since it accomplishes the goal: gabrielchl#1
| import {generateCommonErrorMetadata} from './utils'; | ||
| import {isAutomatedUser as detectAutomatedUser} from './automated-user'; | ||
|
|
||
| const CAMERA_AND_MICROPHONE_PERMISSION_EVENTS = new Set<ClientEvent['name']>([ |
There was a problem hiding this comment.
thought: maybe we could define these constants elsewhere rather than inline in this file. It'd be nice to be able to generate the values they need, but probably more complexity than is needed.
There was a problem hiding this comment.
yeah i think it's probably best to somehow be able to import the type from event dictionary (not necessarily a const but at least it'll give us type checking) but we should prob do that separately
| 'client.call.aborted', | ||
| ]); | ||
|
|
||
| type PermissionResource = keyof PrivacyAndSecurityPermission; |
There was a problem hiding this comment.
nitpick these should be defined in the types file
| type PermissionResource = keyof PrivacyAndSecurityPermission; | ||
| type PermissionState = PrivacyAndSecurityPermission[PermissionResource]; | ||
|
|
||
| const DEFAULT_PERMISSION_SCOPE = 'default'; |
There was a problem hiding this comment.
thought, do we really need this? couldn't we just default the parameter? can we store this in some constants file elsewhere?
|
|
||
| const DEFAULT_PERMISSION_SCOPE = 'default'; | ||
|
|
||
| const isSamePermissionState = (current?: PermissionState, previous?: PermissionState): boolean => |
There was a problem hiding this comment.
nitpick: isEqual ... hehe
| const isSamePermissionState = (current?: PermissionState, previous?: PermissionState): boolean => | ||
| current?.status === previous?.status && current?.reason === previous?.reason; | ||
|
|
||
| const getPermissionResourcesForEvent = ( |
There was a problem hiding this comment.
thought: this code kinda reminds me of the strategy pattern, or chain of responsibility type.
type Payload = RecursivePartial<ClientEvent['payload']>;
type PermissionEnrichmentPolicy = {
resources: readonly PermissionResource[];
terminal: boolean;
};
type PermissionEnrichmentRule = {
events: ReadonlySet<ClientEvent['name']>;
resolve: (payload?: Payload) => PermissionEnrichmentPolicy;
};
const mediaResources = (payload?: Payload): PermissionResource[] => {
switch (payload?.mediaType) {
case 'audio':
return ['microphone'];
case 'video':
return ['camera'];
case 'share':
return ['contentShare'];
default:
return [];
}
};
const PERMISSION_ENRICHMENT_RULES: readonly PermissionEnrichmentRule[] = [
{
events: CAMERA_AND_MICROPHONE_PERMISSION_EVENTS,
resolve: () => ({
resources: ['camera', 'microphone'],
terminal: false,
}),
},
{
events: MEDIA_TX_PERMISSION_EVENTS,
resolve: (payload) => ({
resources: mediaResources(payload),
terminal: false,
}),
},
{
events: CONTENT_SHARE_PERMISSION_EVENTS,
resolve: (payload) => ({
resources: payload?.mediaType === 'share' ? ['contentShare'] : [],
terminal: false,
}),
},
{
events: FINAL_PERMISSION_EVENTS,
resolve: () => ({
resources: ['camera', 'microphone', 'contentShare'],
terminal: true,
}),
},
];
const NO_PERMISSION_ENRICHMENT: PermissionEnrichmentPolicy = {
resources: [],
terminal: false,
};
const resolvePermissionEnrichmentPolicy = (
name: ClientEvent['name'],
payload?: Payload
): PermissionEnrichmentPolicy =>
PERMISSION_ENRICHMENT_RULES.find(({events}) => events.has(name))?.resolve(payload) ??
NO_PERMISSION_ENRICHMENT;
const {resources, terminal: isFinalEvent} =
resolvePermissionEnrichmentPolicy(name, payload);
Something like this maybe? (Ai generated) but I think it formalizes the idea a bit better. To me it reads nicer.
this is, of course, your decision, this code functions perfectly fine as is.
| return []; | ||
| }; | ||
|
|
||
| const projectPrivacyAndSecurityPermission = ( |
There was a problem hiding this comment.
then this becomes:
const policy = resolvePermissionEnrichmentPolicy(name, payload);
const permission = this.privacyAndSecurityPermissionProvider?.();
const projectedPermission = permission
? projectPrivacyAndSecurityPermission(permission, policy.resources)
: undefined;
| * @param args client event name and payload | ||
| * @returns the original or enriched payload | ||
| */ | ||
| private addPrivacyAndSecurityPermission({ |
There was a problem hiding this comment.
and then
class PrivacyAndSecurityPermissionEnricher {
private lastReported = new Map<string, PrivacyAndSecurityPermission>();
private provider?: PrivacyAndSecurityPermissionProvider;
enrich({name, payload, scope}: EnrichmentContext): Payload | undefined {
const policy = resolvePermissionEnrichmentPolicy(name, payload);
try {
// Preserve explicit permissions and record their baseline.
// Obtain the provider snapshot.
// Project the resources selected by the policy.
// Emit only changes, or the complete terminal snapshot.
return enrichedPayload;
} catch (error) {
return payload;
} finally {
if (policy.terminal) {
this.lastReported.delete(scope);
}
}
}
}
| const provider = sinon.stub().returns(permission); | ||
| const payload = {mediaType: 'audio' as const}; | ||
|
|
||
| webex.internal.newMetrics.setPrivacyAndSecurityPermissionProvider(provider); |
There was a problem hiding this comment.
I don't really get the provider bit. why was this done?
There was a problem hiding this comment.
the privacyAndSecurityPermissionProvider is a function that will be provided by cantina, returning the permission statuses on call, it's how the SDK can "ask" the client for the permission statuses
|
|
||
| assert.deepEqual(submittedPayload, { | ||
| mediaType: 'audio', | ||
| privacyAndSecurityPermission: { |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97a54bda89
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return payload; | ||
| } | ||
|
|
||
| this.lastReported.set(scope, {...lastReported, ...changedPermission}); |
There was a problem hiding this comment.
When a changed permission is observed on an event that Call Diagnostics later drops, this line still records it as reported. For example, CallDiagnosticMetrics.shouldSendEvent() limits duplicate client.media.tx.start events per media type/correlation; if the microphone permission changes on a duplicate tx-start, that metric is discarded after enrichment, but lastReported is already advanced, so a later sendable client.media.tx.stop in the same scope omits the changed permission snapshot. Update the history only after the metric is actually queued/sent, or avoid enriching events that will be suppressed.
Useful? React with 👍 / 👎.
| const enrichedPayload = this.privacyAndSecurityPermissionEnricher.enrich({ | ||
| name, | ||
| payload, | ||
| scope: this.getPermissionScope(options), | ||
| }); |
There was a problem hiding this comment.
This enrichment only runs through submitClientEvent, but the public buildClientEventFetchRequestOptions path below still forwards the original payload directly to Call Diagnostics. In the documented before-unload flow for events such as client.call.leave, callers use the fetch-options builder instead of submitClientEvent, so the provider is never invoked, the final permission snapshot is missing, and the scope is not cleared if no normal terminal event is submitted. Apply the same enrichment/scope logic before building fetch options.
Useful? React with 👍 / 👎.
|
closing in favour of #5169 |
2 participants
COMPLETES https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-842561
This pull request addresses
the event definitions were added here: https://sqbu-github.cisco.com/WebExSquared/event-dictionary/pull/2520
by making the following changes
adding metrics to applicable events as defined in https://confluence-eng-gpk2.cisco.com/conf/spaces/CAL/pages/866309853/Daily+Meeting+Issue+Report+Initiative
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.