doc: update metric naming spec #984
Open
+14
−3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
See changes for the new spec when it comes to metric naming. - This will NOT retroactively apply to all existing metrics since this will require significant work refactoring that we do not want to do at the moment. Signed-off-by: nkomonen-amazon <[email protected]>
justinmk3
reviewed
Feb 26, 2025
justinmk3
reviewed
Feb 26, 2025
justinmk3
reviewed
Feb 26, 2025
telemetry/telemetryformat.md
Outdated
| what the metric actually means and how it should be used. | ||
| - `metadata` contains data from `types` that define characteristics of the telemetry beyond | ||
| `createTime` and a `value`. | ||
| - This field is optional, but default `types` are automatically added regardless. |
There was a problem hiding this comment.
I know they are called types but I imagine this will be confusing to read.
Suggested change
| - This field is optional, but default `types` are automatically added regardless. | |
| - This field is optional, but default fields (`types`) are always present on generated metrics. |
justinmk3
reviewed
Feb 26, 2025
telemetry/telemetryformat.md
Outdated
| - Common Namespaces: | ||
| - `toolkit_` is for general non-feature-specific metrics created by the toolkit. Eg: `toolkit_moduleOpen` | ||
| - `ide_` is for IDE specific metrics, not controlled by our extension. Eg: `ide_editorOpen` | ||
| - NOTE: due to legacy reasons the above spec may not be followed, but all future definitions must follow it. |
There was a problem hiding this comment.
Suggested change
| - NOTE: due to legacy reasons the above spec may not be followed, but all future definitions must follow it. | |
| - NOTE: legacy metrics don't always follow the above rules, but new definitions must follow it. |
justinmk3
reviewed
Feb 26, 2025
telemetry/telemetryformat.md
Outdated
Comment on lines
38
to
39
| - put effort to explaining the metrics purpose in detail. This tends to be the source of truth for | ||
| what the metric actually means and how it should be used. |
There was a problem hiding this comment.
Yes. This is also the only way we have to signal "deprecated", currently.
awschristou
reviewed
Feb 26, 2025
| _metrics_ is an array that contains the actual metrics posted to the service. | ||
| - `name` defines the metric name | ||
| - it must be in the format `namespace_camelCaseName` (e.g. `s3_objectUpload`). | ||
| - it must be in the format `{namespace}_{noun}{Verb}` (eg: `toolkit_moduleInit`) |
There was a problem hiding this comment.
Can you add some indented points explaining the rationale? As reviewers we will want to be able to link to this section of the doc to avoid repetition and pushback.
Signed-off-by: nkomonen-amazon <[email protected]>
manodnyab
reviewed
Feb 26, 2025
| - This format enables a natural grouping of related metrics in our definitions file, as well as improved searchability. | ||
| - Common Namespaces: | ||
| - `toolkit_` is for general non-feature-specific metrics created by the toolkit. Eg: `toolkit_moduleOpen` | ||
| - `ide_` is for IDE specific metrics, not controlled by our extension. Eg: `ide_editorOpen` |
There was a problem hiding this comment.
can we also add an entry for ui_click?
There was a problem hiding this comment.
what do you mean specifically?
IIUC we semi-agreed in a previous slack thread that with this new spec, toolkit_uiClick would technically be the name that we go with if it didn't already exist
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
We do not have a clear defined format for metric names.
Solution
Update the doc that defines how to name a metric.
This will NOT retroactively apply to all existing metrics since this will require significant work refactoring that we do not want to do at the moment.
The benefits of this new structure
{namespace}_{noun}{Verb}are that it will group related metrics in the definitions file since they are alphabetically ordered. Also when getting IDE completions all relevant options will be grouped together. This should help for searchability/reusability.License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.