-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(core): Support array attributes for spans, logs and metrics #20427
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: develop
Are you sure you want to change the base?
Changes from 9 commits
74ee2fe
2d84168
fa70b75
aa3b2b1
a71058e
467dfcc
5f62ad7
4b05596
e852120
7cd226a
735bbe8
d9a8974
a26bdc0
50bdc74
cb94756
fa0dbde
c711703
ad724e2
01a6851
20adc34
f896f3a
f76bca2
1538b0d
f24852d
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 |
|---|---|---|
|
|
@@ -15,10 +15,7 @@ type AttributeTypeMap = { | |
| integer: number; | ||
| double: number; | ||
| boolean: boolean; | ||
| 'string[]': Array<string>; | ||
| 'integer[]': Array<number>; | ||
| 'double[]': Array<number>; | ||
| 'boolean[]': Array<boolean>; | ||
| array: Array<string> | Array<number> | Array<boolean>; | ||
| }; | ||
|
|
||
| /* Generates a type from the AttributeTypeMap like: | ||
|
|
@@ -66,9 +63,9 @@ export function isAttributeObject(maybeObj: unknown): maybeObj is AttributeObjec | |
| /** | ||
| * Converts an attribute value to a typed attribute value. | ||
| * | ||
| * For now, we intentionally only support primitive values and attribute objects with primitive values. | ||
| * If @param useFallback is true, we stringify non-primitive values to a string attribute value. Otherwise | ||
| * we return `undefined` for unsupported values. | ||
| * For now, we support primitive values and homogeneous arrays of primitives, either raw or | ||
| * inside attribute objects. If @param useFallback is true, we stringify other non-primitive values | ||
| * to a string attribute value. Otherwise we return `undefined` for unsupported values. | ||
| * | ||
| * @param value - The value of the passed attribute. | ||
| * @param useFallback - If true, unsupported values will be stringified to a string attribute value. | ||
|
|
@@ -170,17 +167,18 @@ function estimatePrimitiveSizeInBytes(value: Primitive): number { | |
| } | ||
|
|
||
| /** | ||
| * NOTE: We intentionally do not return anything for non-primitive values: | ||
| * - array support will come in the future but if we stringify arrays now, | ||
| * sending arrays (unstringified) later will be a subtle breaking change. | ||
| * NOTE: We return typed attributes for primitives and homogeneous arrays of primitives: | ||
| * - Homogeneous primitive arrays ship with `type: 'array'` (Relay's wire tag for arrays). | ||
| * - Mixed-type and nested arrays are not supported and return undefined. | ||
| * - Objects are not supported yet and product support is still TBD. | ||
| * - We still keep the type signature for TypedAttributeValue wider to avoid a | ||
| * breaking change once we add support for non-primitive values. | ||
| * - Once we go back to supporting arrays and stringifying all other values, | ||
| * we already implemented the serialization logic here: | ||
| * https://github.com/getsentry/sentry-javascript/pull/18165 | ||
| * breaking change once we add support for other non-primitive values. | ||
| */ | ||
| function getTypedAttributeValue(value: unknown): TypedAttributeValue | void { | ||
| if (isHomogeneousPrimitiveArray(value)) { | ||
| return { value, type: 'array' }; | ||
| } | ||
|
|
||
| const primitiveType = | ||
| typeof value === 'string' | ||
| ? 'string' | ||
|
|
@@ -201,3 +199,11 @@ function getTypedAttributeValue(value: unknown): TypedAttributeValue | void { | |
| return { value, type: primitiveType }; | ||
| } | ||
| } | ||
|
|
||
| function isHomogeneousPrimitiveArray(arr: unknown): arr is Array<string> | Array<number> | Array<boolean> { | ||
|
Member
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. q: Was it previously decided that SDKs have to guarantee homogeneity? Just wondering if we really have to iterate over the entire array for every array.
Member
Author
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. Jap fair question, we are still discussing that one 😅
Member
Author
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. Talked to ingest: We don't need any SDK-side runtime validation. |
||
| if (!Array.isArray(arr)) return false; | ||
| if (arr.length === 0) return true; | ||
|
nicohrubec marked this conversation as resolved.
Outdated
|
||
| const t = typeof arr[0]; | ||
| if (t !== 'string' && t !== 'number' && t !== 'boolean') return false; | ||
|
nicohrubec marked this conversation as resolved.
Outdated
|
||
| return arr.every(v => typeof v === t); | ||
| } | ||
|
nicohrubec marked this conversation as resolved.
Outdated
|
||
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.
Mixed-type array console parameters silently dropped by Relay
Medium Severity
The
getTypedAttributeValuefunction now converts all non-empty arrays to{ type: 'array' }before the stringify fallback path is reached. This means SDK-internalsentry.message.parameter.Xattributes created bycreateConsoleTemplateAttributesfrom mixed-type console args (e.g.console.log('Array:', [1, 2, 3, 'string'])) are now sent as array attributes instead of being stringified. Since Relay drops non-homogeneous arrays, these template parameters will be silently lost — a regression from the previous behavior where they were preserved as stringified values. The body text is unaffected, but structured template parameter data is lost for mixed-type arrays.Reviewed by Cursor Bugbot for commit f896f3a. Configure here.
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.
this is basically what I described in the PR description, we can discuss it but I think this is a tradeoff we have to make. don't think we can really solve this unless we do runtime inspection of the types of elements in arrays