-
-
Notifications
You must be signed in to change notification settings - Fork 1
chore: Update CONTRIBUTING with process and minor changes #255
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,15 +2,24 @@ | |
|
|
||
| Running this repo requires the usage of [volta](https://volta.sh/). See instructions on installing volta on their documentation [here](https://docs.volta.sh/guide/getting-started). The repo requires Node v22 or higher to run. | ||
|
|
||
| # Setup | ||
| ## Setup | ||
|
|
||
| To install required dependencies, run `yarn install`. | ||
|
|
||
| ```bash | ||
| yarn install | ||
| ``` | ||
|
|
||
| # Usage | ||
| ## Process | ||
|
|
||
| Before attributes are sent from SDKs, or attribute values change, the attributes MUST be defined in this repo. If the convention you need doesn't exist yet, open a PR there to propose it. Only after the convention has been merged, implement it in an SDK. This ensures all SDKs use consistent naming and semantics. | ||
|
|
||
| The merge process for sentry-conventions PRs: | ||
|
|
||
| 1. Open a PR with the proposed convention ([Adding an Attribute](#adding-a-new-attribute)). | ||
| 2. Get an approval from at least one code owner. | ||
| 3. Wait for at least 3 business days after the first approval to give other code owners a chance to review. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. who is gonna be code owner here? |
||
| 4. Merge your PR (alternatively, code owners may merge it after review) | ||
|
|
||
| ## Adding a new attribute | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest to add a paragraph about OTel alignment here. It could look something like this: OTel AlignmentBefore proposing a new attribute, check the OpenTelemetry semantic conventions registry. If OTel already defines the attribute:
If OTel doesn't define it, or the concept is Sentry-specific, set When in doubt, prefer OTel alignment. A Sentry-specific name can be deprecated later; an OTel-aligned name is forward-compatible by default. @Lms24 does that make sense? |
||
|
|
||
|
|
@@ -48,21 +57,22 @@ Remember to run `yarn run generate` after editing or creating a `name` conventio | |
|
|
||
| ## Code and Docs Generation | ||
|
|
||
| After you edit an attribute or add a new one, you should run `yarn run generate && yarn run format` to generate and format the code and docs, which are generated from the json files stored in the `model` directory. | ||
| After you edit an attribute or add a new one, you need to run `yarn run generate` to generate and format the attribute library code, which is generated from the json files stored in the `model` directory. | ||
|
|
||
| # Policies | ||
| ## Policies | ||
|
|
||
| ## Attributes | ||
| ### Attributes | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we add naming rules & examples? Naming rules:
|
||
|
|
||
| Here's a list of policies that any newly added attributes MUST follow. Most of these are automatically enforced by the test suite. | ||
|
|
||
| - The attribute MUST be namespaced. Example: `nextjs.function_id`, not `function_id`. | ||
| - The `pii` field in the attribute definition MUST be `maybe` or `true` (if the attribute can contain sensitive data). It SHOULD be `false` only if scrubbing the attribute value for PII would potentially break product features. For example, `sentry.replay_id` should have `pii` set to `false`. | ||
| - When an attribute is added that deprecates an old one: | ||
| - The old one should be marked as deprecated, and it MUST point to the new one using the `deprecation.replacement` field. | ||
| - For both the new and the old attribute, and any existing aliases of the old attribute, the new and old names MUST be added to the `aliases` list. | ||
| - The deprecation status of the old one SHOULD be set to `backfill` for at least 90 days, and then set to `normalize`. | ||
|
|
||
| # Testing | ||
| ## Testing | ||
|
|
||
| This repo uses [Vitest](https://vitest.dev/) for testing. To run the tests, run `yarn test`. | ||
| The tests enforce logical correctness as well as policies that the model should follow. | ||
|
|
@@ -71,7 +81,7 @@ The tests enforce logical correctness as well as policies that the model should | |
| yarn test | ||
| ``` | ||
|
|
||
| # Linting | ||
| ## Linting | ||
|
|
||
| This repo uses [Biome](https://biome.sh/) and other platform-specific tools for linting. To run the linting, run `yarn lint`. | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imho we should explain somewhere why the 3 day waiting period, something along the lines of
"grace period exists because attribute names, once shipped in an SDK release, are effectively permanent. If a bad name gets adopted by even one SDK, fixing it requires a deprecation cycle across every SDK that shipped it."
we could make it even more stark: "There is no urgency exception if a name feels wrong, raise it during review, not after. "