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(telemetry): Enable telemetry on windows #7389

Merged
merged 7 commits into from
Jan 21, 2023
Merged
26 changes: 25 additions & 1 deletion packages/telemetry/src/sendTelemetry.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import fs from 'fs'
import os from 'os'
import path from 'path'

import { fetch } from '@whatwg-node/fetch'
Expand Down Expand Up @@ -136,7 +137,30 @@ const buildPayload = async () => {
let payload: Record<string, unknown> = {}
let project

const argv = require('yargs/yargs')(process.argv.slice(2)).argv
const processArgv = [...process.argv]

// On windows process.argv may not return an array of strings.
// It will look something like [a,b,c] rather than ["a","b","c"] so we must stringify them before parsing as JSON
// "os.type()" returns 'Windows_NT' on Windows. See https://nodejs.org/docs/latest-v12.x/api/os.html#os_os_type.
if (os.type() === 'Windows_NT') {
Copy link
Member

Choose a reason for hiding this comment

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

I realize this package already used os.type, but the rest of the codebase seems to use process.platform. Any benefit of using one or the other?

Copy link
Collaborator Author

@Josh-Walker-GM Josh-Walker-GM Jan 21, 2023

Choose a reason for hiding this comment

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

Not immediately aware of anything specific other than if we use process.platform it would be one less require? @cannikin had a specific reason behind this choice?

Copy link
Member

Choose a reason for hiding this comment

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

I probably didn't realize there even was a process.platform! Is there anything else telemetry uses from that os lib that isn't available through process?

Also the strings that process.platform returns may be different than what's returned by os? We would want to either convert them to be the same, or update the existing entries in the telemetry database to match, so we don't have different strings for the same platform.

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 think that os is only used for the is windows conditionals.

The actual payload that's sent as telemetry is constructed from data from the envinfo package. That would mean no changes to the actual payload and so no data discontinuities. Unless I'm mistaken?

Copy link
Member

Choose a reason for hiding this comment

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

Sweet! Yeah let's axe it.

Copy link
Member

Choose a reason for hiding this comment

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

I found this https://stackoverflow.com/a/61744810/88106
Seems to suggest os.type() might be more reliable in VMs

Copy link
Collaborator Author

@Josh-Walker-GM Josh-Walker-GM Jan 21, 2023

Choose a reason for hiding this comment

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

The node docs https://nodejs.org/api/os.html#osplatform for os.platform() state that "The return value is equivalent to process.platform."

Although not sure if that SO comment was highlighting that that isn't reliable when operating in VMs.

EDIT: I guess we could just leave it as os.type(). It's worked this long without issues and it appears from the docs that Tobbe's right about being more reliable on VMs.

Copy link
Member

Choose a reason for hiding this comment

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

Turns out I was a genius all along! 😅

const argvIndex = processArgv.findIndex((arg) => arg === '--argv') + 1
let argvFormatted = argvIndex !== 0 ? processArgv[argvIndex] : null
if (argvFormatted) {
argvFormatted =
'[' +
argvFormatted
.substring(1, argvFormatted.length - 1)
.split(',')
.map((arg) => {
return arg.startsWith('"') || arg.startsWith("'") ? arg : `"${arg}"`
})
.join(',') +
']'
processArgv[argvIndex] = argvFormatted
}
}

const argv = require('yargs/yargs')(processArgv.slice(2)).parse()
const rootDir = argv.root
payload = {
type: argv.type || 'command',
Expand Down
43 changes: 27 additions & 16 deletions packages/telemetry/src/telemetry.ts
Original file line number Diff line number Diff line change
@@ -1,23 +1,42 @@
import { spawn } from 'child_process'
import type { SpawnOptions } from 'child_process'
import os from 'os'
import path from 'path'

import { getPaths } from '@redwoodjs/internal/dist/paths'

const spawnProcess = (...args: Array<string>) => {
// "os.type()" returns 'Windows_NT' on Windows. See https://nodejs.org/docs/latest-v12.x/api/os.html#os_os_type.
const execPath =
os.type() === 'Windows_NT' ? `"${process.execPath}"` : process.execPath
const spawnOptions: Partial<SpawnOptions> =
os.type() === 'Windows_NT'
? {
stdio: process.env.REDWOOD_VERBOSE_TELEMETRY
? ['ignore', 'inherit', 'inherit']
: 'ignore',
// The following options run the process in the background without a console window, even though they don't look like they would.
// See https://github.com/nodejs/node/issues/21825#issuecomment-503766781 for information
detached: false,
windowsHide: false,
shell: true,
}
: {
stdio: process.env.REDWOOD_VERBOSE_TELEMETRY
? ['ignore', 'inherit', 'inherit']
: 'ignore',
detached: process.env.REDWOOD_VERBOSE_TELEMETRY ? false : true,
windowsHide: true,
}
spawn(
process.execPath,
execPath,
[
path.join(__dirname, 'scripts', 'invoke.js'),
...args,
'--root',
getPaths().base,
],
{
detached: process.env.REDWOOD_VERBOSE_TELEMETRY ? false : true,
stdio: process.env.REDWOOD_VERBOSE_TELEMETRY ? 'inherit' : 'ignore',
windowsHide: true,
}
spawnOptions
).unref()
}

Expand Down Expand Up @@ -47,14 +66,8 @@ export const timedTelemetry = async (
return result
}

// Returns 'Windows_NT' on Windows.
// See https://nodejs.org/docs/latest-v12.x/api/os.html#os_os_type.
const isWindows = os.type() === 'Windows_NT'

export const errorTelemetry = async (argv: Array<string>, error: any) => {
// FIXME: on Windows, cmd opens and closes a few times.
// See https://github.com/redwoodjs/redwood/issues/5728.
if (isWindows || process.env.REDWOOD_DISABLE_TELEMETRY) {
if (process.env.REDWOOD_DISABLE_TELEMETRY) {
return
}

Expand All @@ -63,9 +76,7 @@ export const errorTelemetry = async (argv: Array<string>, error: any) => {

// used as yargs middleware when any command is invoked
export const telemetryMiddleware = async () => {
// FIXME: on Windows, cmd opens and closes a few times.
// See https://github.com/redwoodjs/redwood/issues/5728.
if (isWindows || process.env.REDWOOD_DISABLE_TELEMETRY) {
if (process.env.REDWOOD_DISABLE_TELEMETRY) {
return
}

Expand Down