-
-
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 21 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 |
|---|---|---|
|
|
@@ -8,17 +8,14 @@ export type RawAttribute<T> = T extends { value: any } | { unit: any } ? Attribu | |
|
|
||
| export type Attributes = Record<string, TypedAttributeValue>; | ||
|
|
||
| export type AttributeValueType = string | number | boolean | Array<string> | Array<number> | Array<boolean>; | ||
| export type AttributeValueType = string | number | boolean | Array<unknown>; | ||
|
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. h: Do we really want
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. Narrowed it down again |
||
|
|
||
| type AttributeTypeMap = { | ||
| string: string; | ||
| integer: number; | ||
| double: number; | ||
| boolean: boolean; | ||
| 'string[]': Array<string>; | ||
| 'integer[]': Array<number>; | ||
| 'double[]': Array<number>; | ||
| 'boolean[]': Array<boolean>; | ||
| array: Array<unknown>; | ||
| }; | ||
|
|
||
| /* 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 arrays, 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. | ||
|
|
@@ -138,21 +135,27 @@ export function estimateTypedAttributesSizeInBytes(attributes: Attributes | unde | |
| return 0; | ||
| } | ||
| let weight = 0; | ||
| const fallbackWeight = 100; | ||
|
|
||
| for (const [key, attr] of Object.entries(attributes)) { | ||
| weight += key.length * 2; | ||
| weight += attr.type.length * 2; | ||
| weight += (attr.unit?.length ?? 0) * 2; | ||
| const val = attr.value; | ||
|
|
||
| if (Array.isArray(val)) { | ||
| if (Array.isArray(val) && isPrimitive(val[0])) { | ||
| // Assumption: Individual array items have the same type and roughly the same size | ||
| // probably not always true but allows us to cut down on runtime | ||
| weight += estimatePrimitiveSizeInBytes(val[0]) * val.length; | ||
| } else if (Array.isArray(val) && !isPrimitive(val[0])) { | ||
|
sentry[bot] marked this conversation as resolved.
Outdated
|
||
| // Fallback for arrays with non-primitive values (e.g. objects) | ||
| // Again (to cut down on runtime), we intentionally don't traverse the full hierarchy | ||
| weight += fallbackWeight * val.length; | ||
| } else if (isPrimitive(val)) { | ||
| weight += estimatePrimitiveSizeInBytes(val); | ||
| } else { | ||
| // default fallback for anything else (objects) | ||
| weight += 100; | ||
| weight += fallbackWeight; | ||
| } | ||
| } | ||
| return weight; | ||
|
|
@@ -170,17 +173,15 @@ 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 arrays: | ||
| * - Relay currently only supports arrays consisting of primitive values. Attributes with non-conforming arrays are dropped by Relay, so runtime type validation in the SDK is unnecessary. | ||
| * - 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 | ||
| */ | ||
| function getTypedAttributeValue(value: unknown): TypedAttributeValue | void { | ||
| if (Array.isArray(value) && value.length !== 0) { | ||
| return { value, type: 'array' }; | ||
|
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. Mixed-type array console parameters silently dropped by RelayMedium Severity The Reviewed by Cursor Bugbot for commit f896f3a. Configure here.
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. 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 |
||
| } | ||
|
|
||
| const primitiveType = | ||
| typeof value === 'string' | ||
| ? 'string' | ||
|
|
||


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.
Thanks for pulling this one in.