Validate that telemetry definitions are ordered alphabetically; Reorder the current file #705
Conversation
| @@ -0,0 +1,26 @@ | |||
| name: Lint Telemetry Definitions | |||
There was a problem hiding this comment.
could be just "Lint", all linting can be in one CI place
| name: Lint Telemetry Definitions | |
| name: Lint |
There was a problem hiding this comment.
I'd prefer to parallelize all of the linting, having each job focus on its own domain. We only have the one thing being linted in CI, and it's easy to change course though. Let's leave it as-is for now.
telemetry/validation/src/fix.ts
Outdated
| await saveTelemetryDefinitions(definitions, jsonPath) | ||
| } | ||
|
|
||
| export function fix(definitions: TelemetryDefinitions): void { |
There was a problem hiding this comment.
Seems like we're re-inventing eslint. We should probably implement this as an eslint plugin, otherwise it's a novel system that others must learn, for not much reason.
vscode toolkit has custom eslint plugins here: https://github.com/aws/aws-toolkit-vscode/tree/master/plugins/eslint-plugin-aws-toolkits
There was a problem hiding this comment.
Apparently its a little different to make eslint rules on json. I started out that way, and opted to just write pure scripts.
|
|
||
| if (inputMetric.name != sortedName) { | ||
| validations.push(`Telemetry Metrics are not sorted. Expected: ${sortedName}, Found: ${inputMetric.name}`) | ||
| break |
There was a problem hiding this comment.
should we post msg that they can fix by command?
perhaps adding run npm run fix
There was a problem hiding this comment.
ohh my bad.. I see it's already added in final msg
There was a problem hiding this comment.
This still seems a bit over-engineered. A validation script shouldn't need a package.json, it could be plain javascript. This will add a dependency-maintenance cost too, besides the more general risk that it's another project that will accumulate quirks and hidden corners.
eslint plugins do require a sub-project. But with this solution we're gaining a project but not gaining eslint :/ (Assuming that eslint is even needed; if we are only "linting" json we don't need eslint, or a test framework...)
Problem
Metrics are defined in haphazard order in the telemetry definitions file. This makes it challenging to locate entries, particularly those sharing a common prefix. Maintaining sorted lists also makes it easier to reason about entries being added or modified, without having to think about their location.
Solution
Scripting has been created to:
A CI job has been set up to run the validation check during PRs
This change was not wired up as a commit hook, because the repo does not have a top level node project. The CI job, and the readme are the mechanisms used to maintain the file going forward.
I chose not to use eslint. I started out in that direction, but it was becoming more of a distraction from the problem being solved. From https://github.com/azeemba/eslint-plugin-json?tab=readme-ov-file#is-eslint-really-the-best-tool-to-lint-my-json:
I ran the fix script against the current definitions file. This PR also contains that change as a separate commit.
To verify this change, I ran the new fix script on the current telemetry file. I then hand sorted the same file, and verified that the resulting file matched what the script produced.
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.