Skip to content
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ sentryTest('should capture console object calls', async ({ getLocalTestUrl, page
'sentry.message.template': { value: 'Mixed: {} {} {} {}', type: 'string' },
'sentry.message.parameter.0': { value: 'prefix', type: 'string' },
'sentry.message.parameter.1': { value: '{"obj":true}', type: 'string' },
'sentry.message.parameter.2': { value: '[4,5,6]', type: 'string' },
'sentry.message.parameter.2': { value: [4, 5, 6], type: 'array' },
'sentry.message.parameter.3': { value: 'suffix', type: 'string' },
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ Sentry.logger.info('log_before_any_scope', { log_attr: 'log_attr_1' });

Sentry.getGlobalScope().setAttributes({ global_scope_attr: true });

// this attribute will not be sent for now
Sentry.getGlobalScope().setAttribute('array_attr', [1, 2, 3]);

// global scope, log attribute
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ sentryTest('captures logs with scope attributes', async ({ getLocalTestUrl, page
'sentry.sdk.version': { value: expect.any(String), type: 'string' },
'sentry.timestamp.sequence': { value: expect.any(Number), type: 'integer' },
global_scope_attr: { value: true, type: 'boolean' },
array_attr: { value: [1, 2, 3], type: 'array' },
log_attr: { value: 'log_attr_2', type: 'string' },
},
},
Expand All @@ -61,6 +62,7 @@ sentryTest('captures logs with scope attributes', async ({ getLocalTestUrl, page
'sentry.sdk.version': { value: expect.any(String), type: 'string' },
'sentry.timestamp.sequence': { value: expect.any(Number), type: 'integer' },
global_scope_attr: { value: true, type: 'boolean' },
array_attr: { value: [1, 2, 3], type: 'array' },
isolation_scope_1_attr: { value: 100, unit: 'millisecond', type: 'integer' },
log_attr: { value: 'log_attr_3', type: 'string' },
},
Expand All @@ -76,6 +78,7 @@ sentryTest('captures logs with scope attributes', async ({ getLocalTestUrl, page
'sentry.sdk.version': { value: expect.any(String), type: 'string' },
'sentry.timestamp.sequence': { value: expect.any(Number), type: 'integer' },
global_scope_attr: { value: true, type: 'boolean' },
array_attr: { value: [1, 2, 3], type: 'array' },
isolation_scope_1_attr: { value: 100, unit: 'millisecond', type: 'integer' },
scope_attr: { value: 200, unit: 'millisecond', type: 'integer' },
log_attr: { value: 'log_attr_4', type: 'string' },
Expand All @@ -92,6 +95,7 @@ sentryTest('captures logs with scope attributes', async ({ getLocalTestUrl, page
'sentry.sdk.version': { value: expect.any(String), type: 'string' },
'sentry.timestamp.sequence': { value: expect.any(Number), type: 'integer' },
global_scope_attr: { value: true, type: 'boolean' },
array_attr: { value: [1, 2, 3], type: 'array' },
isolation_scope_1_attr: { value: 100, unit: 'millisecond', type: 'integer' },
scope_2_attr: { value: 300, unit: 'millisecond', type: 'integer' },
log_attr: { value: 'log_attr_5', type: 'string' },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ async function run(): Promise<void> {

Sentry.getGlobalScope().setAttribute('global_scope_attr', true);

// this attribute will not be sent for now
Sentry.getGlobalScope().setAttributes({ array_attr: [1, 2, 3] });

// global scope, log attribute
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ describe('logs', () => {
attributes: {
...commonAttributes,
global_scope_attr: { value: true, type: 'boolean' },
array_attr: { value: [1, 2, 3], type: 'array' },
log_attr: { value: 'log_attr_2', type: 'string' },
},
},
Expand All @@ -72,6 +73,7 @@ describe('logs', () => {
attributes: {
...commonAttributes,
global_scope_attr: { value: true, type: 'boolean' },
array_attr: { value: [1, 2, 3], type: 'array' },
isolation_scope_1_attr: { value: 100, unit: 'millisecond', type: 'integer' },
log_attr: { value: 'log_attr_3', type: 'string' },
},
Expand All @@ -85,6 +87,7 @@ describe('logs', () => {
attributes: {
...commonAttributes,
global_scope_attr: { value: true, type: 'boolean' },
array_attr: { value: [1, 2, 3], type: 'array' },
isolation_scope_1_attr: { value: 100, unit: 'millisecond', type: 'integer' },
scope_attr: { value: 200, unit: 'millisecond', type: 'integer' },
log_attr: { value: 'log_attr_4', type: 'string' },
Expand All @@ -99,6 +102,7 @@ describe('logs', () => {
attributes: {
...commonAttributes,
global_scope_attr: { value: true, type: 'boolean' },
array_attr: { value: [1, 2, 3], type: 'array' },
isolation_scope_1_attr: { value: 100, unit: 'millisecond', type: 'integer' },
scope_2_attr: { value: 300, unit: 'millisecond', type: 'integer' },
log_attr: { value: 'log_attr_5', type: 'string' },
Expand Down
34 changes: 20 additions & 14 deletions packages/core/src/attributes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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' };
Copy link
Copy Markdown

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 getTypedAttributeValue function now converts all non-empty arrays to { type: 'array' } before the stringify fallback path is reached. This means SDK-internal sentry.message.parameter.X attributes created by createConsoleTemplateAttributes from 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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit f896f3a. Configure here.

Copy link
Copy Markdown
Member Author

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

}

const primitiveType =
typeof value === 'string'
? 'string'
Expand All @@ -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> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jap fair question, we are still discussing that one 😅

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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;
Comment thread
nicohrubec marked this conversation as resolved.
Outdated
const t = typeof arr[0];
if (t !== 'string' && t !== 'number' && t !== 'boolean') return false;
Comment thread
nicohrubec marked this conversation as resolved.
Outdated
return arr.every(v => typeof v === t && (t !== 'number' || !Number.isNaN(v)));
}
Comment thread
nicohrubec marked this conversation as resolved.
Outdated
90 changes: 44 additions & 46 deletions packages/core/test/lib/attributes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,33 +76,38 @@ describe('attributeValueToTypedAttributeValue', () => {
);
});

describe('invalid values (non-primitives)', () => {
it.each([
['foo', 'bar'],
[1, 2, 3],
[true, false, true],
[1, 'foo', true],
{ foo: 'bar' },
() => 'test',
Symbol('test'),
])('returns undefined for non-primitive raw values (%s)', value => {
const result = attributeValueToTypedAttributeValue(value);
expect(result).toBeUndefined();
});
describe('homogeneous primitive arrays', () => {
it.each([[['foo', 'bar']], [[1, 2, 3]], [[true, false, true]], [[] as unknown[]]])(
'emits a typed array attribute for raw value %j',
value => {
const result = attributeValueToTypedAttributeValue(value);
expect(result).toStrictEqual({ value, type: 'array' });
},
);

it.each([
['foo', 'bar'],
[1, 2, 3],
[true, false, true],
[1, 'foo', true],
{ foo: 'bar' },
() => 'test',
Symbol('test'),
])('returns undefined for non-primitive attribute object values (%s)', value => {
const result = attributeValueToTypedAttributeValue({ value });
expect(result).toBeUndefined();
it('emits a typed array attribute for attribute object values', () => {
const result = attributeValueToTypedAttributeValue({ value: ['foo', 'bar'] });
expect(result).toStrictEqual({ value: ['foo', 'bar'], type: 'array' });
});
});

describe('invalid values (non-primitives)', () => {
it.each([[[1, 'foo', true]], [[NaN, 1, 2]], [{ foo: 'bar' }], [() => 'test'], [Symbol('test')]])(
'returns undefined for non-primitive raw values (%s)',
value => {
const result = attributeValueToTypedAttributeValue(value);
expect(result).toBeUndefined();
},
);

it.each([[[1, 'foo', true]], [[NaN, 1, 2]], [{ foo: 'bar' }], [() => 'test'], [Symbol('test')]])(
'returns undefined for non-primitive attribute object values (%s)',
value => {
const result = attributeValueToTypedAttributeValue({ value });
expect(result).toBeUndefined();
},
);
});
});

describe('with fallback=true', () => {
Expand Down Expand Up @@ -189,26 +194,10 @@ describe('attributeValueToTypedAttributeValue', () => {
});

describe('invalid values (non-primitives) - stringified fallback', () => {
it('stringifies string arrays', () => {
const result = attributeValueToTypedAttributeValue(['foo', 'bar'], true);
it('stringifies mixed-type arrays (not homogeneous)', () => {
const result = attributeValueToTypedAttributeValue(['foo', 1, true], true);
expect(result).toStrictEqual({
value: '["foo","bar"]',
type: 'string',
});
});

it('stringifies number arrays', () => {
const result = attributeValueToTypedAttributeValue([1, 2, 3], true);
expect(result).toStrictEqual({
value: '[1,2,3]',
type: 'string',
});
});

it('stringifies boolean arrays', () => {
const result = attributeValueToTypedAttributeValue([true, false, true], true);
expect(result).toStrictEqual({
value: '[true,false,true]',
value: '["foo",1,true]',
type: 'string',
});
});
Expand Down Expand Up @@ -425,15 +414,17 @@ describe('serializeAttributes', () => {
describe('invalid (non-primitive) values', () => {
it("doesn't fall back to stringification by default", () => {
const result = serializeAttributes({ foo: { some: 'object' }, bar: [1, 2, 3], baz: () => {} });
expect(result).toStrictEqual({});
expect(result).toStrictEqual({
bar: { type: 'array', value: [1, 2, 3] },
});
});

it('falls back to stringification of unsupported non-primitive values if fallback is true', () => {
const result = serializeAttributes({ foo: { some: 'object' }, bar: [1, 2, 3], baz: () => {} }, true);
expect(result).toStrictEqual({
bar: {
type: 'string',
value: '[1,2,3]',
type: 'array',
value: [1, 2, 3],
},
baz: {
type: 'string',
Expand All @@ -445,5 +436,12 @@ describe('serializeAttributes', () => {
},
});
});

it('drops mixed-type arrays by default and stringifies them with fallback', () => {
expect(serializeAttributes({ mixed: ['a', 1] })).toStrictEqual({});
expect(serializeAttributes({ mixed: ['a', 1] }, true)).toStrictEqual({
mixed: { type: 'string', value: '["a",1]' },
});
});
});
});
10 changes: 9 additions & 1 deletion packages/core/test/lib/logs/internal.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,6 @@ describe('_INTERNAL_captureLog', () => {
scope.setAttribute('scope_2', { value: 38, unit: 'gigabyte' });
scope.setAttributes({
scope_3: true,
// these are invalid since for now we don't support arrays
scope_4: [1, 2, 3],
scope_5: { value: [true, false, true], unit: 'second' },
});
Expand Down Expand Up @@ -229,6 +228,15 @@ describe('_INTERNAL_captureLog', () => {
type: 'boolean',
value: true,
},
scope_4: {
type: 'array',
value: [1, 2, 3],
},
scope_5: {
type: 'array',
value: [true, false, true],
unit: 'second',
},
'sentry.timestamp.sequence': { value: expect.any(Number), type: 'integer' },
});
});
Expand Down
6 changes: 3 additions & 3 deletions packages/core/test/lib/tracing/spans/estimateSize.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,9 @@ describe('estimateSerializedSpanSizeInBytes', () => {
status: 'ok',
is_segment: false,
attributes: {
'item.ids': { type: 'string[]', value: ['id-001', 'id-002', 'id-003', 'id-004', 'id-005'] },
scores: { type: 'double[]', value: [1.1, 2.2, 3.3, 4.4] },
flags: { type: 'boolean[]', value: [true, false, true] },
'item.ids': { type: 'array', value: ['id-001', 'id-002', 'id-003', 'id-004', 'id-005'] },
scores: { type: 'array', value: [1.1, 2.2, 3.3, 4.4] },
flags: { type: 'array', value: [true, false, true] },
},
};

Expand Down
4 changes: 1 addition & 3 deletions packages/core/test/lib/utils/spanUtils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -622,11 +622,9 @@ describe('spanToJSON', () => {
attr1: { type: 'string', value: 'value1' },
attr2: { type: 'integer', value: 2 },
attr3: { type: 'boolean', value: true },
attr4: { type: 'array', value: [1, 2, 3] },
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: { type: 'string', value: 'test op' },
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: { type: 'string', value: 'auto' },
// notice the absence of `attr4`!
// for now, we don't yet serialize array attributes. This test will fail
// once we allow serializing them.
},
links: [
{
Expand Down
Loading