-
Notifications
You must be signed in to change notification settings - Fork 89
VSCODE-20: setup telemetry #52
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
Conversation
070f334
to
e45a201
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. one small comment, good to go. * Edit - have we checked that these events show up in segment? One thought is that we could require that the bundling step errors when there is no segment key, to help safeguard if we ever move to a different CI and forget to add it in or run a manual packaging for distribution without the key. Might be not necessary though.
const mockStorageController = new StorageController(mockExtensionContext); | ||
|
||
test('get segment key from constants keyfile', () => { | ||
const testPlaygroundController = new TelemetryController(mockStorageController); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small thing - testTelemetryController
instead of testPlaygroundController
- I think down below too
package.json
Outdated
"mdb.sendTelemetry": { | ||
"type": "boolean", | ||
"default": true, | ||
"description": "Allow to send diagnostic and usage telemetry data." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another possible description
: Allow the sending of diagnostic and usage telemetry data to help improve user experience.
const ui = ora('Generate constants keyfile').start(); | ||
await writeFile( | ||
`${ROOT_DIR}/constants.json`, | ||
JSON.stringify({ segmentKey: process.env.SEGMENT_KEY }, null, 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just clarifying so I understand - Is the reason for creating the constants.json
file so that we can use it in the built extension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid storing segment key on git, we keep il locally in .env file, that is not being pushed to git, and during the compilation process, we generate constants.json file with this key injected. The constants.json file is also added to .gitignore
, so the key exists only on the local machine.
To publish vscode extension or to build a package locally we use vsce
command-line tool, sort of npm publish
for vscode, that require the compilation step as well, therefore the build will also include constants.json file.
When we run tests on Azure, we don't have access to .env file that is stored on our computer, therefore to get constants.json generated there, I use Azure Pipeline variable that I pass to npm test script in azure-pipelines.yml.
I think later when we will have automatic publishing using CI we can get rid of .env file and pass a the same azure variable to deploy script. Since we don't publish to the marketplace yet, and I can't test how this key injection will work, I have this intermediate solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we can keep .env, in any case, if we want to continue building packages locally, what also can be useful.
@Anemy yes Max confirmed that data gets to Segment. About throwing an error I think it is a good idea to throw it during compilation time if the key is missing in environment. Will update the generate-keyfile.ts script. |
lgtm nice work |
Description
playground code executed
we track the type of returned bymongosh
result, which can be one of these:[query|aggregation|insert|update|delete|other]
.async_hooks
don't work as expected with electron on mac, what causes tests failing for all PRs. It can be caused by a current version of electron itself or by our data service. As a workaround I'm settingNODE_OPTIONS="--no-force-async-hooks-checks"
env variable. The ticker for further investigation: https://jira.mongodb.org/browse/COMPASS-4087Checklist
Motivation and Context
Types of changes