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

fix: abridge call graph creation error messages in analytics #1279

Merged
merged 1 commit into from
Jul 22, 2020
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
12 changes: 12 additions & 0 deletions src/lib/error-format.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
export function abridgeErrorMessage(
msg: string,
maxLen: number,
ellipsis?: string,
): string {
if (msg.length <= maxLen) {
return msg;
}
const e = ellipsis || ' ... ';

Choose a reason for hiding this comment

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

Maybe have it as an optional param on line 4?
ellipsis = '...': string,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that supported by TypeScript?

Choose a reason for hiding this comment

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

Optional parameters? in JS and TS

const toKeep = (maxLen - e.length) / 2;

Choose a reason for hiding this comment

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

I think toKeep can be a not whole number

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know. But it seems to round it when used in slice.

Choose a reason for hiding this comment

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

I think it would be better to round it before, less JS magic

return msg.slice(0, toKeep) + e + msg.slice(msg.length - toKeep, msg.length);
}
11 changes: 10 additions & 1 deletion src/lib/monitor/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,12 @@ import {
} from './utils';
import { countPathsToGraphRoot } from '../utils';
import * as alerts from '../alerts';
import { abridgeErrorMessage } from '../error-format';

const debug = Debug('snyk');

const ANALYTICS_PAYLOAD_MAX_LENGTH = 1024;

// TODO(kyegupov): clean up the type, move to snyk-cli-interface repository

interface MonitorBody {
Expand Down Expand Up @@ -220,7 +223,13 @@ async function monitorDepTree(
let callGraphPayload;
if (options.reachableVulns && scannedProject.callGraph?.innerError) {
const err = scannedProject.callGraph as CallGraphError;
analytics.add('callGraphError', err.innerError.toString());
analytics.add(
'callGraphError',
abridgeErrorMessage(

Choose a reason for hiding this comment

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

This should go also to the test file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Testing that method would be quite involved, and the functional tests for abridging the string should cover a reasonable number of cases.

Choose a reason for hiding this comment

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

I think it would be nice to say why we are doing this trimming with a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is pretty self-explanatory from the code, no? E.g. ANALYTICS_PAYLOAD_MAX_LENGTH

err.innerError.toString(),
ANALYTICS_PAYLOAD_MAX_LENGTH,
),
);
alerts.registerAlerts([
{
type: 'error',
Expand Down
11 changes: 10 additions & 1 deletion src/lib/snyk-test/run-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,12 @@ import { assembleIacLocalPayloads, parseIacTestResult } from './run-iac-test';
import { Payload, PayloadBody, DepTreeFromResolveDeps } from './types';
import { CallGraphError } from '@snyk/cli-interface/legacy/common';
import * as alerts from '../alerts';
import { abridgeErrorMessage } from '../error-format';

const debug = debugModule('snyk');

const ANALYTICS_PAYLOAD_MAX_LENGTH = 1024;

export = runTest;

async function sendAndParseResults(
Expand Down Expand Up @@ -520,7 +523,13 @@ async function assembleLocalPayloads(
(scannedProject.callGraph as CallGraphError).innerError
) {
const err = scannedProject.callGraph as CallGraphError;
analytics.add('callGraphError', err.innerError.toString());
analytics.add(
'callGraphError',
abridgeErrorMessage(
err.innerError.toString(),
ANALYTICS_PAYLOAD_MAX_LENGTH,
),
);
alerts.registerAlerts([
{
type: 'error',
Expand Down
22 changes: 22 additions & 0 deletions test/error-format.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { test } from 'tap';
import { abridgeErrorMessage } from '../src/lib/error-format';

test('abridge empty string', async (t) => {
muscar marked this conversation as resolved.
Show resolved Hide resolved
t.equal(abridgeErrorMessage('', 10), '');
});

test('abridge shorter than max length', async (t) => {
t.equal(abridgeErrorMessage('hello', 10), 'hello');
});

test('abridge same length as max length', async (t) => {
t.equal(abridgeErrorMessage('hello', 5), 'hello');
});

test('abridge longer than max length', async (t) => {
t.equal(abridgeErrorMessage('hello there', 10), 'he ... ere');
});

test('abridge longer than max length (custom ellipsis)', async (t) => {
t.equal(abridgeErrorMessage('hello there', 10, '--'), 'hell--here');
});