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] Address package name limitations #14252

Closed

Conversation

danielayala94
Copy link
Contributor

@danielayala94 danielayala94 commented Jan 7, 2025

Description

Fix to ensure that telemetry package names contain only the following characters: alphanumeric, underscore, dot. Also, make sure it starts with letter, and it has 100 characters or less.

Resolves #14218

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

Why

The telemetry backend doesn't like that some of the packages contain characters like @ and -. This is problematic when trying to create dimensions that depend on the version of packages like:

react-native-windows
@react-native-community/cli

What

  1. Move nameHelpers module from CLI to telemetry.
  2. Added a condition to process package names in case it's not compliant with the rules described previously.
  3. Introduced a couple of functions in nameHelpers to verify and process package names. Replace all "bad" characters with underscores.
  4. Moved nameHelpers tests to its own file, plus added test cases for the new nameHelpers functions.

Screenshots

N/A

Testing

Added new test cases to nameHelpers, and verified in the telemetry backend that all package versions are uploaded.

Changelog

No

Microsoft Reviewers: Open in CodeFlow

return false;
}

export function cleanTelemetryPackageName(str: string): string {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that it affects any of the packages, but there is no check to trim the package name if it's over 100 characters,

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth putting it in though, if it's an actual requirement.

}
"include": [
"src",
"../telemetry/src/utils/nameHelpers.ts"
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't want to cross include packages, like this, don't include this here. You won't need it if you properly export the nameHelpers functions from the telemetry package, and remember to re-build the telemetry package so you can see them in this package.

@@ -18,7 +18,7 @@ import {
findPropertyValue,
tryFindPropertyValueAsBoolean,
} from '../commands/config/configUtils';
import * as nameHelpers from '../utils/nameHelpers';
import * as nameHelpers from '@react-native-windows/telemetry/src/utils/nameHelpers';
Copy link
Contributor

Choose a reason for hiding this comment

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

See the above comment for properly exporting the functions you need and consuming them here.

@@ -29,7 +29,7 @@ import {
endTelemetrySession,
} from '../../utils/telemetryHelpers';
import {copyAndReplaceWithChangedCallback} from '../../generator-common';
import * as nameHelpers from '../../utils/nameHelpers';
import * as nameHelpers from '@react-native-windows/telemetry/src/utils/nameHelpers';
Copy link
Contributor

Choose a reason for hiding this comment

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

Look at telemetry/index.ts to see how to export the functions we need there (isValidProjectNamespace, isValidProjectName, cleanName, cleanNamespace). Then you can import those functions here with:

Suggested change
import * as nameHelpers from '@react-native-windows/telemetry/src/utils/nameHelpers';
import {isValidProjectNamespace, isValidProjectName, cleanName, cleanNamespace} from '@react-native-windows/telemetry';

return false;
}

export function cleanTelemetryPackageName(str: string): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth putting it in though, if it's an actual requirement.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) label Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Telemetry Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace dashes in telemetry property names
2 participants