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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 7 commits into from
Aug 13, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
da07424
Make stats and client logs more easily accessible through slash commands
streamer45 Aug 7, 2024
61348bf
Util function
streamer45 Aug 12, 2024
f40649b
Command response wrapper
streamer45 Aug 12, 2024
7710471
Fix e2e
streamer45 Aug 12, 2024
915b0f5
Revert "Fix e2e"
streamer45 Aug 12, 2024
54750ae
Use Playbooks v2
streamer45 Aug 12, 2024
ced5539
Merge remote-tracking branch 'origin/main' into MM-59996
streamer45 Aug 13, 2024
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
View file
Open in desktop
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');
});
});
30 changes: 30 additions & 0 deletions server/slash_command.go
View file
Open in desktop
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 @@ -245,6 +264,17 @@ func (p *Plugin) ExecuteCommand(_ *plugin.Context, args *model.CommandArgs) (*mo
return resp, nil
}

if subCmd == logsCommandTrigger {
Copy link

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. :)

Copy link

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.

Copy link

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.

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

if subCmd == endCommandTrigger {
resp, err := p.handleEndCallCommand()
if err != nil {
Expand Down
23 changes: 15 additions & 8 deletions webapp/src/client.ts
View file
Open in desktop
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 {logDebug, logErr, logInfo, logWarn, persistClientLogs} from './log';
import {getScreenStream} 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,8 @@ export default class CallsClient extends EventEmitter {
this.closed = true;
if (this.peer) {
this.getStats().then((stats) => {
sessionStorage.setItem('calls_client_stats', JSON.stringify(stats));
const storage = window.desktop ? localStorage : sessionStorage;
enzowritescode marked this conversation as resolved.
Show resolved Hide resolved
storage.setItem(STORAGE_CALLS_CLIENT_STATS_KEY, JSON.stringify(stats));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't want to overcomplicate it by adding listeners, so the stats are saved when the client disconnects (user leaves call) and can be retrieved after that.

This slightly differs from Web browser behavior, where stats can be fetched at any time during or after a call, but the simple fix is enough to cover the customer's use case (troubleshooting).

}).catch((statsErr) => {
logErr(statsErr);
});
Expand Down
7 changes: 7 additions & 0 deletions webapp/src/constants.ts
View file
Open in desktop
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';
38 changes: 36 additions & 2 deletions webapp/src/log.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,54 @@
/* eslint-disable no-console */

import {STORAGE_CALLS_CLIENT_LOGS_KEY} from 'src/constants';

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() {
const storage = window.desktop ? localStorage : sessionStorage;
storage.setItem(STORAGE_CALLS_CLIENT_LOGS_KEY, clientLogs);
clientLogs = '';
}

export function getClientLogs() {
const storage = window.desktop ? localStorage : sessionStorage;
return storage.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);
}
}
18 changes: 13 additions & 5 deletions webapp/src/slash_commands.tsx
View file
Open in desktop
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 {
channelHasCall,
channelIDForCurrentCall,
Expand Down Expand Up @@ -147,11 +151,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 @@ -163,9 +167,13 @@ export default async function slashCommandsHandler(store: Store, joinCall: joinC
return {error: {message: err}};
}
}
const data = sessionStorage.getItem('calls_client_stats') || '{}';
const storage = window.desktop ? localStorage : sessionStorage;
const data = storage.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
3 changes: 2 additions & 1 deletion webapp/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {getCurrentUserId} from 'mattermost-redux/selectors/entities/users';
import {IntlShape} from 'react-intl';
import {parseSemVer} from 'semver-parser';
import CallsClient from 'src/client';
import {STORAGE_CALLS_EXPERIMENTAL_FEATURES_KEY} from 'src/constants';
import RestClient from 'src/rest_client';
import {notificationSounds} from 'src/webapp_globals';

Expand Down Expand Up @@ -334,7 +335,7 @@ export function setSDPMaxVideoBW(sdp: string, bandwidth: number) {
}

export function hasExperimentalFlag() {
return window.localStorage.getItem('calls_experimental_features') === 'on';
return window.localStorage.getItem(STORAGE_CALLS_EXPERIMENTAL_FEATURES_KEY) === 'on';
}

export function split<T>(list: T[], i: number, pad = false): [list: T[], overflowed?: T[]] {
Expand Down
Loading