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

[MM-59996] Make stats and client logs more easily accessible through slash commands #840

Merged
merged 7 commits into from
Aug 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions e2e/tests/desktop.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -238,4 +238,47 @@ test.describe('desktop', () => {
// Verify error is getting sent
expect(desktopAPICalls.leaveCall).toBe(true);
});

test('desktop: /call stats command', async ({page}) => {
// start call in global widget
const devPage = new PlaywrightDevPage(page);
await devPage.openWidget(getChannelNamesForTest()[0]);
await devPage.leaveCall();

// Need to wait a moment since the the leave call happens in
// a setTimeout handler.
await devPage.wait(500);

// Go back to center channel view
await devPage.goto();

// Issue slash command
await devPage.sendSlashCommand('/call stats');
await devPage.wait(500);

// Veirfy call stats have been returned
await expect(page.locator('.post__body').last()).toContainText('"initTime"');
await expect(page.locator('.post__body').last()).toContainText('"callID"');
});

test('desktop: /call logs command', async ({page}) => {
// start call in global widget
const devPage = new PlaywrightDevPage(page);
await devPage.openWidget(getChannelNamesForTest()[0]);
await devPage.leaveCall();

// Need to wait a moment since the the leave call happens in
// a setTimeout handler.
await devPage.wait(500);

// Go back to center channel view
await devPage.goto();

// Issue slash command
await devPage.sendSlashCommand('/call logs');
await devPage.wait(500);

// Veirfy call logs have been returned
await expect(page.locator('.post__body').last()).toContainText('join ack received, initializing connection');
});
});
75 changes: 33 additions & 42 deletions server/slash_command.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const (
endCommandTrigger = "end"
recordingCommandTrigger = "recording"
hostCommandTrigger = "host"
logsCommandTrigger = "logs"
)

var subCommands = []string{
Expand All @@ -36,6 +37,7 @@ var subCommands = []string{
endCommandTrigger,
statsCommandTrigger,
recordingCommandTrigger,
logsCommandTrigger,
}

func (p *Plugin) getAutocompleteData() *model.AutocompleteData {
Expand All @@ -49,6 +51,7 @@ func (p *Plugin) getAutocompleteData() *model.AutocompleteData {
data.AddCommand(model.NewAutocompleteData(linkCommandTrigger, "", "Generate a link to join a call in the current channel."))
data.AddCommand(model.NewAutocompleteData(statsCommandTrigger, "", "Show client-generated statistics about the call."))
data.AddCommand(model.NewAutocompleteData(endCommandTrigger, "", "End the call for everyone. All the participants will drop immediately."))
data.AddCommand(model.NewAutocompleteData(logsCommandTrigger, "", "Show client logs."))

experimentalCmdData := model.NewAutocompleteData(experimentalCommandTrigger, "", "Turn experimental features on or off.")
experimentalCmdData.AddTextArgument("Available options: on, off", "", "on|off")
Expand Down Expand Up @@ -157,6 +160,22 @@ func handleStatsCommand(fields []string) (*model.CommandResponse, error) {
}, nil
}

func handleLogsCommand(fields []string) (*model.CommandResponse, error) {
if len(fields) < 3 {
return nil, fmt.Errorf("Empty logs")
}

logs, err := base64.StdEncoding.DecodeString(fields[2])
if err != nil {
return nil, fmt.Errorf("Failed to decode payload: %w", err)
}

return &model.CommandResponse{
ResponseType: model.CommandResponseTypeEphemeral,
Text: fmt.Sprintf("```\n%s\n```", logs),
}, nil
}

func (p *Plugin) handleEndCallCommand() (*model.CommandResponse, error) {
return &model.CommandResponse{}, nil
}
Expand Down Expand Up @@ -212,8 +231,7 @@ func (p *Plugin) ExecuteCommand(_ *plugin.Context, args *model.CommandArgs) (*mo

subCmd := fields[1]

if subCmd == linkCommandTrigger {
resp, err := p.handleLinkCommand(args)
buildCommandResponse := func(resp *model.CommandResponse, err error) (*model.CommandResponse, *model.AppError) {
if err != nil {
return &model.CommandResponse{
ResponseType: model.CommandResponseTypeEphemeral,
Expand All @@ -223,59 +241,32 @@ func (p *Plugin) ExecuteCommand(_ *plugin.Context, args *model.CommandArgs) (*mo
return resp, nil
}

if subCmd == linkCommandTrigger {
return buildCommandResponse(p.handleLinkCommand(args))
}

if subCmd == experimentalCommandTrigger {
resp, err := handleExperimentalCommand(fields)
if err != nil {
return &model.CommandResponse{
ResponseType: model.CommandResponseTypeEphemeral,
Text: fmt.Sprintf("Error: %s", err.Error()),
}, nil
}
return resp, nil
return buildCommandResponse(handleExperimentalCommand(fields))
}

if subCmd == statsCommandTrigger {
resp, err := handleStatsCommand(fields)
if err != nil {
return &model.CommandResponse{
ResponseType: model.CommandResponseTypeEphemeral,
Text: fmt.Sprintf("Error: %s", err.Error()),
}, nil
}
return resp, nil
return buildCommandResponse(handleStatsCommand(fields))
}

if subCmd == logsCommandTrigger {

Choose a reason for hiding this comment

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

This if subCmd == subCommandCXyz { ... } format in this function is getting kind of long.
Either in this PR or another can we shorten it?

Maybe something like the following (or a switch):

if subCmd == subCommandXyz {
return subCommandXyzHelper()
}

Copy link
Member

Choose a reason for hiding this comment

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

On the other hand, having a long function that reads simply top-to-bottom can be easier and quicker to read and understand, compared to having to jump back and forth to/from functions. So there's an argument for keeping it simple, too. :)

Choose a reason for hiding this comment

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

@cpoile single responsibility principle + easier to write tests for / understand the code coverage, especially if you use an IDE tool that shows highlights for code coverage.

I'm fine with leaving it as is if nobody else thinks it's an issue, but it's definitely a code smell.

Choose a reason for hiding this comment

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

Another option is I can make this change if you all want in another ticket.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@enzowritescode I added a wrapping utility to clean it up a little. Let me know if that works.

return buildCommandResponse(handleLogsCommand(fields))
}

if subCmd == endCommandTrigger {
resp, err := p.handleEndCallCommand()
if err != nil {
return &model.CommandResponse{
ResponseType: model.CommandResponseTypeEphemeral,
Text: fmt.Sprintf("Error: %s", err.Error()),
}, nil
}
return resp, nil
return buildCommandResponse(p.handleEndCallCommand())
}

if subCmd == recordingCommandTrigger {
resp, err := p.handleRecordingCommand(fields)
if err != nil {
return &model.CommandResponse{
ResponseType: model.CommandResponseTypeEphemeral,
Text: fmt.Sprintf("Error: %s", err.Error()),
}, nil
}
return resp, nil
return buildCommandResponse(p.handleRecordingCommand(fields))
}

if subCmd == hostCommandTrigger && p.licenseChecker.HostControlsAllowed() {
resp, err := p.handleHostCommand(args, fields)
if err != nil {
return &model.CommandResponse{
ResponseType: model.CommandResponseTypeEphemeral,
Text: fmt.Sprintf("Error: %s", err.Error()),
}, nil
}
return resp, nil
return buildCommandResponse(p.handleHostCommand(args, fields))
}

for _, cmd := range subCommands {
Expand Down
24 changes: 15 additions & 9 deletions webapp/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,14 @@ import {EventEmitter} from 'events';
import {deflate} from 'pako/lib/deflate';
import {AudioDevices, CallsClientConfig, CallsClientStats, TrackInfo} from 'src/types/types';

import {logDebug, logErr, logInfo, logWarn} from './log';
import {getScreenStream} from './utils';
import {logDebug, logErr, logInfo, logWarn, persistClientLogs} from './log';
import {getScreenStream, getPersistentStorage} from './utils';
import {WebSocketClient, WebSocketError, WebSocketErrorType} from './websocket';
import {
STORAGE_CALLS_CLIENT_STATS_KEY,
STORAGE_CALLS_DEFAULT_AUDIO_INPUT_KEY,
STORAGE_CALLS_DEFAULT_AUDIO_OUTPUT_KEY,
} from 'src/constants';

export const AudioInputPermissionsError = new Error('missing audio input permissions');
export const AudioInputMissingError = new Error('no audio input available');
Expand Down Expand Up @@ -99,8 +104,8 @@ export default class CallsClient extends EventEmitter {
};
}

const defaultInputID = window.localStorage.getItem('calls_default_audio_input');
const defaultOutputID = window.localStorage.getItem('calls_default_audio_output');
const defaultInputID = window.localStorage.getItem(STORAGE_CALLS_DEFAULT_AUDIO_INPUT_KEY);
Copy link
Member

Choose a reason for hiding this comment

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

👍

const defaultOutputID = window.localStorage.getItem(STORAGE_CALLS_DEFAULT_AUDIO_OUTPUT_KEY);
if (defaultInputID && !this.currentAudioInputDevice) {
const devices = this.audioDevices.inputs.filter((dev) => {
return dev.deviceId === defaultInputID;
Expand All @@ -114,7 +119,7 @@ export default class CallsClient extends EventEmitter {
this.currentAudioInputDevice = devices[0];
} else {
logDebug('audio input device not found');
window.localStorage.removeItem('calls_default_audio_input');
window.localStorage.removeItem(STORAGE_CALLS_DEFAULT_AUDIO_INPUT_KEY);
}
}

Expand All @@ -128,7 +133,7 @@ export default class CallsClient extends EventEmitter {
this.currentAudioOutputDevice = devices[0];
} else {
logDebug('audio output device not found');
window.localStorage.removeItem('calls_default_audio_output');
window.localStorage.removeItem(STORAGE_CALLS_DEFAULT_AUDIO_OUTPUT_KEY);
}
}

Expand Down Expand Up @@ -407,14 +412,15 @@ export default class CallsClient extends EventEmitter {
this.removeAllListeners('mos');
window.removeEventListener('beforeunload', this.onBeforeUnload);
navigator.mediaDevices?.removeEventListener('devicechange', this.onDeviceChange);
persistClientLogs();
}

public async setAudioInputDevice(device: MediaDeviceInfo) {
if (!this.peer) {
return;
}

window.localStorage.setItem('calls_default_audio_input', device.deviceId);
window.localStorage.setItem(STORAGE_CALLS_DEFAULT_AUDIO_INPUT_KEY, device.deviceId);
this.currentAudioInputDevice = device;

// We emit this event so it's easier to keep state in sync between widget and pop out.
Expand Down Expand Up @@ -464,7 +470,7 @@ export default class CallsClient extends EventEmitter {
if (!this.peer) {
return;
}
window.localStorage.setItem('calls_default_audio_output', device.deviceId);
window.localStorage.setItem(STORAGE_CALLS_DEFAULT_AUDIO_OUTPUT_KEY, device.deviceId);
this.currentAudioOutputDevice = device;

// We emit this event so it's easier to keep state in sync between widget and pop out.
Expand All @@ -484,7 +490,7 @@ export default class CallsClient extends EventEmitter {
this.closed = true;
if (this.peer) {
this.getStats().then((stats) => {
sessionStorage.setItem('calls_client_stats', JSON.stringify(stats));
getPersistentStorage().setItem(STORAGE_CALLS_CLIENT_STATS_KEY, JSON.stringify(stats));
}).catch((statsErr) => {
logErr(statsErr);
});
Expand Down
7 changes: 7 additions & 0 deletions webapp/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,10 @@ export const CallTranscribingDisclaimerStrings: {[key: string]: {[key: string]:
export const DisabledCallsErr = new Error('Cannot start or join call: calls are disabled in this channel.');

export const supportedLocales = [];

// Local/Session storage keys
export const STORAGE_CALLS_CLIENT_STATS_KEY = 'calls_client_stats';
export const STORAGE_CALLS_CLIENT_LOGS_KEY = 'calls_client_logs';
export const STORAGE_CALLS_DEFAULT_AUDIO_INPUT_KEY = 'calls_default_audio_input';
export const STORAGE_CALLS_DEFAULT_AUDIO_OUTPUT_KEY = 'calls_default_audio_output';
export const STORAGE_CALLS_EXPERIMENTAL_FEATURES_KEY = 'calls_experimental_features';
37 changes: 35 additions & 2 deletions webapp/src/log.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,53 @@
/* eslint-disable no-console */

import {STORAGE_CALLS_CLIENT_LOGS_KEY} from 'src/constants';
import {getPersistentStorage} from 'src/utils';

import {pluginId} from './manifest';

let clientLogs = '';
Copy link
Member

Choose a reason for hiding this comment

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

❤️ js


function appendClientLog(level: string, ...args: unknown[]) {
clientLogs += `${level} [${new Date().toISOString()}] ${args}\n`;
}

export function persistClientLogs() {
getPersistentStorage().setItem(STORAGE_CALLS_CLIENT_LOGS_KEY, clientLogs);
clientLogs = '';
}

export function getClientLogs() {
return getPersistentStorage().getItem(STORAGE_CALLS_CLIENT_LOGS_KEY) || '';
}

export function logErr(...args: unknown[]) {
console.error(`${pluginId}:`, ...args);
try {
if (window.callsClient) {
appendClientLog('error', ...args);
}
} catch (err) {
console.error(err);
}
}

export function logWarn(...args: unknown[]) {
console.warn(`${pluginId}:`, ...args);
if (window.callsClient) {
appendClientLog('warn', ...args);
}
}

export function logInfo(...args: unknown[]) {
console.info(`${pluginId}:`, ...args);
if (window.callsClient) {
appendClientLog('info', ...args);
}
}

export function logDebug(...args: unknown[]) {
// TODO: convert to debug once we are out of beta.
console.info(`${pluginId}:`, ...args);
console.debug(`${pluginId}:`, ...args);
if (window.callsClient) {
appendClientLog('debug', ...args);
}
}
19 changes: 13 additions & 6 deletions webapp/src/slash_commands.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,14 @@ import {
stopCallRecording,
trackEvent,
} from 'src/actions';
import {DisabledCallsErr} from 'src/constants';
import {
DisabledCallsErr,
STORAGE_CALLS_CLIENT_STATS_KEY,
STORAGE_CALLS_EXPERIMENTAL_FEATURES_KEY,
} from 'src/constants';
import * as Telemetry from 'src/types/telemetry';

import {logDebug} from './log';
import {getClientLogs, logDebug} from './log';
import {
areGroupCallsAllowed,
channelHasCall,
Expand All @@ -25,7 +29,7 @@ import {
isRecordingInCurrentCall,
} from './selectors';
import {Store} from './types/mattermost-webapp';
import {getCallsClient, isDMChannel, sendDesktopEvent, shouldRenderDesktopWidget} from './utils';
import {getCallsClient, getPersistentStorage, isDMChannel, sendDesktopEvent, shouldRenderDesktopWidget} from './utils';

type joinCallFn = (channelId: string, teamId?: string, title?: string, rootId?: string) => void;

Expand Down Expand Up @@ -158,11 +162,11 @@ export default async function slashCommandsHandler(store: Store, joinCall: joinC
break;
}
if (fields[2] === 'on') {
window.localStorage.setItem('calls_experimental_features', 'on');
window.localStorage.setItem(STORAGE_CALLS_EXPERIMENTAL_FEATURES_KEY, 'on');
logDebug('experimental features enabled');
} else if (fields[2] === 'off') {
logDebug('experimental features disabled');
window.localStorage.removeItem('calls_experimental_features');
window.localStorage.removeItem(STORAGE_CALLS_EXPERIMENTAL_FEATURES_KEY);
}
break;
case 'stats': {
Expand All @@ -174,9 +178,12 @@ export default async function slashCommandsHandler(store: Store, joinCall: joinC
return {error: {message: err}};
}
}
const data = sessionStorage.getItem('calls_client_stats') || '{}';
const data = getPersistentStorage().getItem(STORAGE_CALLS_CLIENT_STATS_KEY) || '{}';
return {message: `/call stats ${btoa(data)}`, args};
}
case 'logs': {
return {message: `/call logs ${btoa(getClientLogs())}`, args};
}
case 'recording': {
if (fields.length < 3 || (fields[2] !== 'start' && fields[2] !== 'stop')) {
break;
Expand Down
Loading
Loading