-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
ref(develop/scopes): Remove attribute type as a user-provided property
#15522
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
ref(develop/scopes): Remove attribute type as a user-provided property
#15522
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
| - The SDK SHOULD keep the attribute format consistent with the user-set format until user-provided processing callbacks like `before_send_log` have been called. This ensures compatibility with already existing callbacks and avoids unexpected changes to the attribute format. | ||
|
|
||
| See [Span Protocol - Common Attribute Keys](/sdk/telemetry/spans/span-protocol/#common-attribute-keys) for a list of standard attributes and [Sentry Conventions](https://github.com/getsentry/sentry-conventions/) for the complete attribute registry. | ||
|
|
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.
Bug: New requirement breaks existing before_send_log callbacks by changing expected attribute format, removing the type field.
Severity: CRITICAL | Confidence: 0.95
🔍 Detailed Analysis
The new requirement in scopes.mdx at line 103, stating that SDKs should maintain the user-set attribute format until before_send_log callbacks are called, introduces a breaking change. Existing user callbacks expect attributes to be in a typed object format, including a type field (e.g., { "attribute_name": { type: "string", value: "xyz" } }). However, the new requirement allows primitive values (e.g., { "attribute_name": true }) or typed objects without a type field (e.g., { "attribute_name": { value: 3600, unit: "s" } }) to be passed to these callbacks. This will cause existing callbacks to malfunction when attempting to access attribute.type or attribute.value, or when modifying attributes based on the previously expected structure.
💡 Suggested Fix
Revert the requirement at scopes.mdx:103 or clarify that SDKs must convert user-set attributes to the full typed object format (including type field) before passing them to before_send_log and similar callbacks to maintain compatibility.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: develop-docs/sdk/telemetry/scopes.mdx#L103-L106
Potential issue: The new requirement in `scopes.mdx` at line 103, stating that SDKs
should maintain the user-set attribute format until `before_send_log` callbacks are
called, introduces a breaking change. Existing user callbacks expect attributes to be in
a typed object format, including a `type` field (e.g., `{ "attribute_name": { type:
"string", value: "xyz" } }`). However, the new requirement allows primitive values
(e.g., `{ "attribute_name": true }`) or typed objects without a `type` field (e.g., `{
"attribute_name": { value: 3600, unit: "s" } }`) to be passed to these callbacks. This
will cause existing callbacks to malfunction when attempting to access `attribute.type`
or `attribute.value`, or when modifying attributes based on the previously expected
structure.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2689521
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.
that's exactly what I want to avoid :D Not sure how other languages built beforeSendLog. If JS is rather the exception than the norm here, that's fine. But we only serialize to the more complex, typed log object after beforeSendLog. So users generally expect to find the same key-value structure in beforeSendLong as the one they used to set attributes in the first place
This is a proposal after working on scope attributes for JS. We don't have to merge this but IMHO it makes sense.
This PR makes a slight change to the scope attributes specification:
When users want to set a scope attribute with a unit, the previous spec currently required them to also pass
typeexplicitly.This PR changes to spec to preferrably remove the
typeproperty from the attribute object in favour of letting the SDK internally check and set the correct type. Two advantages:This is a SHOULD, so if SDKs are constrained in some way, they can still require the user to pass the type explicitly.
Furthermore, this PR also updates a recommendation to leave set attribute format as-is, so that users can consume the attributes in
beforeSendLogand other callbacks, in the same structure as they are initially set. This again is a SHOULD because most importantly, it must be compatible with how attributes are consumed in said callbacks today already.