fix mac address resolution on touchbar devices

[sbatten]

• Fixes an issue with mac addresses on MBP with TouchBar.
• Removes unnecessary dependencies shipped to get mac address of machine
• Sends an event to help understand machines with this issue

[bpasero]

@sbatten lots of changes are needed, I am not sure if you maybe asked for a review too early before finishing this PR? There is a "Draft" feature for PRs to handle those cases for the future.

Besides my feedback, you also seem to miss test coverage from id.test.ts where we still seem to test the old getmac solution and not your new solution.

Are we at risk of changing our machine ID resolution to a new system that is potentially unsafe? Or was the getmac module doing the same thing?

//cc @kieferrm given the potential impact of this change on our telemetry

[bpasero]

This really is hard to read, I suggest to rewrite the 2 methods using async:

let machineId: Promise<string>;
export async function getMachineId(): Promise<string> {
	if (!machineId) {
		machineId = (async () => {
			const id = await getMacAddress();

			return id || uuid.generateUuid(); // fallback, generate a UUID
		})();
	}

	return machineId;
}

async function getMacAddress(): Promise<string | undefined> {
	try {
		const crypto = await import('crypto');
		const macAddress = await getMac();

		return crypto.createHash('sha256').update(macAddress, 'utf8').digest('hex');
	} catch (error) {
		errors.onUnexpectedError(error);
		return undefined;
	}
}

[bpasero]

We no longer seem to depend on getmac. Why does this PR not include the necessary changes to remove getmac from our package.json, yarn.lock, getmac.d.ts, etc.?

[bpasero]

I am not sure we can trust the timeout parameter. It makes sense to me to have it, but I would also have the timeout mechanism we used to have before where we would resolve the promise after 10s automatically instead of relying on the OS to do this.

[bpasero]

I am super confused now by this code which seems to resolve the machine ID twice depending on some value of the machine ID. I suggest to do this all from within ONE method and not two that does the right thing depending on the state. E.g. resolveMachineId should return the "true" machine ID in case needed.

[sbatten]

@bpasero we need to resolve 2 separate machined ids, not just one. We should always return and use the originally calculated machine id as machineId. TrueMachineId is intended only as a disambiguation in this specific case.

[bpasero]

Oh, only now saw that @kieferrm is also on the review list 👍

[sbatten]

@bpasero, the intent of this change is to maintain the existing calculation. we should not see a change in mac address resolution.

[bpasero]

@sbatten I pushed some changes:

• the timeout was never cleared (f4ad2d0)
• the resolution of machine ID is now in one method (c5cbffd)

I still think we can improve this: there is now a machineId and a trueMachineId floating around in app.ts. I find that very hard to understand by just reading the code. I suggest to introduce an interface IMachineId { original: string, resolved: string } that we pass around. As you can see, machineId is being used in many places (also shared process) but the new trueMachineId is only used in one place it seems. How can I know which ID to use in which case?

An alternative and even better solution would be: replace the machineId with the newly resolved machineId if it matches 6c9d2bc8f91b89624add29c0abeae7fb42bf539fa1cdb2e3e57cd668fa9bcead. I see no big value in using the machine ID 6c9d2bc8f91b89624add29c0abeae7fb42bf539fa1cdb2e3e57cd668fa9bcead for telemetry if so many mac users might have it?

E.g. it then becomes:

private async resolveMachineId(): Promise<string> {

	// We cache the machineId for faster lookups on startup
	// and resolve it only once initially if not cached
	let machineId = this.stateService.getItem<string>(CodeApplication.MACHINE_ID_KEY);
	if (isMacintosh && machineId === '6c9d2bc8f91b89624add29c0abeae7fb42bf539fa1cdb2e3e57cd668fa9bcead') {
		machineId = undefined;
	}

	if (!machineId) {
		machineId = await getMachineId();

		this.stateService.setItem(CodeApplication.MACHINE_ID_KEY, machineId);
	}

	return machineId;
}

(probably would need to store some state flag to prevent always resolving the machine ID if it always resolves to 6c9d2bc8f91b89624add29c0abeae7fb42bf539fa1cdb2e3e57cd668fa9bcead)

[sbatten]

@kieferrm discussed with ben offline so this is ready to merge, PTAL