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

[Telemetry] Migration from AppInsights to 1DS #13493

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

danielayala94
Copy link
Contributor

@danielayala94 danielayala94 commented Jul 24, 2024

Description

Type of Change

New feature.

Why

Enable posting telemetry data to have information that will allow us to take informed decisions on what areas of RNW we should focus, identify customer pains, RNW usage, etc.

Resolves #10473
Resolves #10142
Resolves #10141

What

  • Refactored Telemetry class, by changing all App Insights SDK references to 1DS core+post APIs. The most noticeable changes are:
    • Replaced TelemetryClient with AppInsightsCore.
    • Refactored post event logic to craft a telemetry item object.
    • Manually parsed stack trace into decomposed frames, as 1DS doesn't provide such functionality (which AppInsights does).
  • Refactored some of the testing code to account for the changes in the Telemetry class.

Testing

All E2E tests on telemetry.test.ts are passing after the refactoring.
All other telemetry tests are passing after the refactoring as well.

Changelog

Yes

Microsoft Reviewers: Open in CodeFlow

@danielayala94 danielayala94 marked this pull request as ready for review August 23, 2024 22:38
@danielayala94 danielayala94 requested review from a team as code owners August 23, 2024 22:38
@danielayala94 danielayala94 changed the title [Draft, Telemetry] Migration from AppInsights to 1DS Core [Telemetry] Migration from AppInsights to 1DS Aug 23, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) and removed Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) labels Aug 26, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) and removed Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) labels Sep 5, 2024
*/
export async function fullBuildInfo(): Promise<string> {
try {
let regCommand = `${process.env.windir}\\System32\\reg.exe query ${DeviceIdBuildPath} /v ${DeviceIdBuildKey}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you refactor the command part of this into a registry-key getting helper that can be shared with deviceId()?

@@ -67,12 +67,6 @@ test('deviceDiskFreeSpace() is valid', () => {
expect(value).toBeGreaterThanOrEqual(0);
});

test('sampleRate() is within valid range', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the telemetry automatically contain the sample rate?

* @returns A string containing the Windows build name, number and architecture.
* e.g. 19569.1000.amd64fre.rs_prerelease.200214-1419
*/
export async function fullBuildInfo(): Promise<string> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make sure to add a test for this?

@@ -63,7 +89,9 @@ export function nodeArchitecture(): string {
* @returns The device platform.
*/
export function devicePlatform(): string {
return platform();
const os = platform();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep this function behavior as-is and rename it to nodePlatform()?

I want to make sure we record what node's platform() call returns (since it's our of our control and may be relevant to debugging issues) and would rather stick it in our own common field rather than munge it to fit into the PartA deviceClass.

Then we should then have a different method to get the correct value for deviceClass (which may take the output from nodePlatform and transform it to Windows for windows PCs, but we should make sure we also put the correct expected values for mac/linux).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants