Skip to content

Commit

Permalink
Enhance telemetry data and exclude tensorboard tracebacks (#1172)
Browse files Browse the repository at this point in the history
* Enrich analytics with container information and options

* Add reminder

* Avoid stack trace leak on Tensorboard

* Make the code self-mindblowing

* Pin `is-docker` in `package.json` 🙀

* Update tests

* Fix double `.options`

* Simplify container

* Fix tests, again

* Simplify telemetry options

* Fix tests, again

* Restore name/full differentiation

* Revert telemetryData

* Fix legacy commands

* Surely not meant to be here

* Apply suggestions from code review

Co-authored-by: Domas Monkus <[email protected]>

* Unpack yargs in function body

* fixup! Unpack yargs in function body

* Add a Helpful Comment™

* Fix merge

* Apply suggestions from code review

Co-authored-by: Domas Monkus <[email protected]>
  • Loading branch information
0x2b3bfa0 and tasdomas authored Oct 11, 2022
1 parent c1ec211 commit 188cde1
Show file tree
Hide file tree
Showing 17 changed files with 124 additions and 31 deletions.
2 changes: 2 additions & 0 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,8 @@ WORKDIR ${CML_RUNNER_PATH}
# SET SPECIFIC ENVIRONMENT VARIABLES
ENV IN_DOCKER=1
ENV RUNNER_ALLOW_RUNASROOT=1
# Environment variable used by cml to detect it's been installed using the docker image.
ENV _CML_CONTAINER_IMAGE=true

# DEFINE ENTRY POINT AND COMMAND
# Smart entrypoint understands commands like `bash` or `/bin/sh` but defaults to `cml`;
Expand Down
28 changes: 24 additions & 4 deletions bin/cml.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ const setupOpts = (opts) => {

const { markdownfile } = opts;
opts.markdownFile = markdownfile;
opts.cmlCommand = opts._[0];
opts.cml = new CML(opts);
};

Expand Down Expand Up @@ -76,9 +75,29 @@ const setupLogger = (opts) => {
});
};

const setupTelemetry = async (opts) => {
const { cml, cmlCommand: action } = opts;
opts.telemetryEvent = await jitsuEventPayload({ action, cml });
const setupTelemetry = async (opts, yargs) => {
const { cml, _: command } = opts;

const options = {};
for (const [name, option] of Object.entries(opts.options)) {
// Skip options with default values (i.e. not explicitly set by users)
if (opts[name] && !yargs.parsed.defaulted[name]) {
switch (option.telemetryData) {
case 'name':
options[name] = null;
break;
case 'full':
options[name] = opts[name];
break;
}
}
}

opts.telemetryEvent = await jitsuEventPayload({
action: command.join(':'),
extra: { options },
cml
});
};

const runPlugin = async ({ $0: executable, command }) => {
Expand All @@ -99,6 +118,7 @@ const handleError = (message, error) => {

(async () => {
setupLogger({ log: 'debug' });

try {
await yargs
.env('CML')
Expand Down
9 changes: 7 additions & 2 deletions bin/cml/asset/publish.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,11 @@ exports.handler = async (opts) => {
else await fs.writeFile(file, output);
};

exports.builder = (yargs) => yargs.env('CML_ASSET').options(exports.options);
exports.builder = (yargs) =>
yargs
.env('CML_ASSET')
.option('options', { default: exports.options, hidden: true })
.options(exports.options);

exports.options = kebabcaseKeys({
url: {
Expand Down Expand Up @@ -51,7 +55,8 @@ exports.options = kebabcaseKeys({
},
rmWatermark: {
type: 'boolean',
description: 'Avoid CML watermark.'
description: 'Avoid CML watermark.',
telemetryData: 'name'
},
mimeType: {
type: 'string',
Expand Down
6 changes: 5 additions & 1 deletion bin/cml/check/create.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,11 @@ exports.handler = async (opts) => {
await cml.checkCreate({ ...opts, report });
};

exports.builder = (yargs) => yargs.env('CML_CHECK').options(exports.options);
exports.builder = (yargs) =>
yargs
.env('CML_CHECK')
.option('options', { default: exports.options, hidden: true })
.options(exports.options);

exports.options = kebabcaseKeys({
token: {
Expand Down
15 changes: 11 additions & 4 deletions bin/cml/comment/create.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@ exports.handler = async (opts) => {
console.log(await cml.commentCreate(opts));
};

exports.builder = (yargs) => yargs.env('CML_COMMENT').options(exports.options);
exports.builder = (yargs) =>
yargs
.env('CML_COMMENT')
.option('options', { default: exports.options, hidden: true })
.options(exports.options);

exports.options = kebabcaseKeys({
pr: {
Expand All @@ -30,7 +34,8 @@ exports.options = kebabcaseKeys({
publishUrl: {
type: 'string',
default: 'https://asset.cml.dev',
description: 'Self-hosted image server URL'
description: 'Self-hosted image server URL',
telemetryData: 'name'
},
watch: {
type: 'boolean',
Expand All @@ -44,7 +49,8 @@ exports.options = kebabcaseKeys({
native: {
type: 'boolean',
description:
"Uses driver's native capabilities to upload assets instead of CML's storage; not available on GitHub"
"Uses driver's native capabilities to upload assets instead of CML's storage; not available on GitHub",
telemetryData: 'name'
},
update: {
type: 'boolean',
Expand All @@ -55,6 +61,7 @@ exports.options = kebabcaseKeys({
rmWatermark: {
type: 'boolean',
description:
'Avoid watermark; CML needs a watermark to be able to distinguish CML comments from others'
'Avoid watermark; CML needs a watermark to be able to distinguish CML comments from others',
telemetryData: 'name'
}
});
1 change: 1 addition & 0 deletions bin/cml/pr.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,6 @@ exports.builder = (yargs) =>
])
)
)
.option('options', { default: options, hidden: true })
.check(({ globpath }) => globpath)
.strict();
6 changes: 5 additions & 1 deletion bin/cml/pr/create.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@ exports.handler = async (opts) => {
console.log(link);
};

exports.builder = (yargs) => yargs.env('CML_PR').options(exports.options);
exports.builder = (yargs) =>
yargs
.env('CML_PR')
.option('options', { default: exports.options, hidden: true })
.options(exports.options);

exports.options = kebabcaseKeys({
md: {
Expand Down
6 changes: 5 additions & 1 deletion bin/cml/repo/prepare.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,11 @@ exports.handler = async (opts) => {
await cml.ci(opts);
};

exports.builder = (yargs) => yargs.env('CML_REPO').options(exports.options);
exports.builder = (yargs) =>
yargs
.env('CML_REPO')
.option('options', { default: exports.options, hidden: true })
.options(exports.options);

exports.options = kebabcaseKeys({
unshallow: {
Expand Down
1 change: 1 addition & 0 deletions bin/cml/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,6 @@ exports.builder = (yargs) =>
])
)
)
.option('options', { default: options, hidden: true })
.check(() => process.argv.some((arg) => arg.startsWith('-')))
.strict();
21 changes: 15 additions & 6 deletions bin/cml/runner/launch.js
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,11 @@ exports.handler = async (opts) => {
}
};

exports.builder = (yargs) => yargs.env('CML_RUNNER').options(exports.options);
exports.builder = (yargs) =>
yargs
.env('CML_RUNNER')
.option('options', { default: exports.options, hidden: true })
.options(exports.options);

exports.options = kebabcaseKeys({
labels: {
Expand Down Expand Up @@ -454,13 +458,15 @@ exports.options = kebabcaseKeys({
type: 'boolean',
conflicts: ['single', 'reuseIdle'],
description:
"Don't launch a new runner if an existing one has the same name or overlapping labels"
"Don't launch a new runner if an existing one has the same name or overlapping labels",
telemetryData: 'name'
},
reuseIdle: {
type: 'boolean',
conflicts: ['reuse', 'single'],
description:
"Creates a new runner only if the matching labels don't exist or are already busy"
"Creates a new runner only if the matching labels don't exist or are already busy",
telemetryData: 'name'
},
workdir: {
type: 'string',
Expand All @@ -476,7 +482,8 @@ exports.options = kebabcaseKeys({
cloud: {
type: 'string',
choices: ['aws', 'azure', 'gcp', 'kubernetes'],
description: 'Cloud to deploy the runner'
description: 'Cloud to deploy the runner',
telemetryData: 'full'
},
cloudRegion: {
type: 'string',
Expand Down Expand Up @@ -529,12 +536,14 @@ exports.options = kebabcaseKeys({
type: 'number',
default: -1,
description:
'Maximum spot instance bidding price in USD. Defaults to the current spot bidding price'
'Maximum spot instance bidding price in USD. Defaults to the current spot bidding price',
telemetryData: 'name'
},
cloudStartupScript: {
type: 'string',
description:
'Run the provided Base64-encoded Linux shell script during the instance initialization'
'Run the provided Base64-encoded Linux shell script during the instance initialization',
telemetryData: 'name'
},
cloudAwsSecurityGroup: {
type: 'string',
Expand Down
2 changes: 1 addition & 1 deletion bin/cml/tensorboard/connect.e2e.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ describe('tbLink', () => {
error = err;
}

expect(error.message).toBe(`Tensorboard took too long. ${message}`);
expect(error.message).toBe(`Tensorboard took too long`);
});

test('valid url is returned', async () => {
Expand Down
17 changes: 11 additions & 6 deletions bin/cml/tensorboard/connect.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const kebabcaseKeys = require('kebabcase-keys');
const { spawn } = require('child_process');
const { homedir } = require('os');
const tempy = require('tempy');
const winston = require('winston');

const { exec, watermarkUri, sleep } = require('../../../src/utils');

Expand All @@ -28,8 +29,8 @@ const tbLink = async (opts = {}) => {
chrono = chrono + chronoStep;
}

const error = await fs.readFile(stderror, 'utf8');
throw new Error(`Tensorboard took too long. ${error}`);
winston.error(await fs.readFile(stderror, 'utf8'));
throw new Error(`Tensorboard took too long`);
};

const launchAndWaitLink = async (opts = {}) => {
Expand All @@ -51,8 +52,8 @@ const launchAndWaitLink = async (opts = {}) => {
proc.unref();
proc.on('exit', async (code, signal) => {
if (code || signal) {
const error = await fs.readFile(stderrPath, 'utf8');
throw new Error(`Tensorboard failed with error: ${error}`);
winston.error(await fs.readFile(stderrPath, 'utf8'));
throw new Error(`Tensorboard failed with error ${code || signal}`);
}
});

Expand Down Expand Up @@ -95,7 +96,10 @@ exports.handler = async (opts) => {
};

exports.builder = (yargs) =>
yargs.env('CML_TENSORBOARD').options(exports.options);
yargs
.env('CML_TENSORBOARD')
.option('options', { default: exports.options, hidden: true })
.options(exports.options);

exports.options = kebabcaseKeys({
credentials: {
Expand Down Expand Up @@ -136,6 +140,7 @@ exports.options = kebabcaseKeys({
},
rmWatermark: {
type: 'boolean',
description: 'Avoid CML watermark'
description: 'Avoid CML watermark',
telemetryData: 'name'
}
});
6 changes: 5 additions & 1 deletion bin/cml/workflow/rerun.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@ exports.handler = async (opts) => {
await cml.pipelineRerun(opts);
};

exports.builder = (yargs) => yargs.env('CML_WORKFLOW').options(exports.options);
exports.builder = (yargs) =>
yargs
.env('CML_WORKFLOW')
.option('options', { default: exports.options, hidden: true })
.options(exports.options);

exports.options = kebabcaseKeys({
id: {
Expand Down
20 changes: 20 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@
"git-url-parse": "^13.1.0",
"globby": "^11.0.4",
"https-proxy-agent": "^5.0.1",
"is-docker": "2.2.1",
"js-base64": "^3.7.2",
"kebabcase-keys": "^1.0.0",
"node-fetch": "^2.6.5",
Expand Down
5 changes: 3 additions & 2 deletions src/analytics.e2e.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ describe('analytics tests', () => {
const cml = new CML({ repo: REPO, token: TOKEN });
const action = 'test';
const cloud = 'azure';
const container = 'cml';
const more = { one: 1, two: 2 };
const extra = { cloud, ...more };
const extra = { cloud, container, ...more };
const error = 'Ouch!';
const regex = /\d+\.\d+\.\d+/;

Expand All @@ -25,7 +26,7 @@ describe('analytics tests', () => {
expect(pl.backend).toBe(cloud);
expect(pl.error).toBe(error);
expect(Object.keys(pl.extra).sort()).toEqual(
['ci'].concat(Object.keys(more)).sort()
['ci', 'container'].concat(Object.keys(more)).sort()
);

if (isCI()) {
Expand Down
Loading

0 comments on commit 188cde1

Please sign in to comment.