Uh oh!
There was an error while loading. Please reload this page.
feat(core): Extract objects as structured logs in consoleLoggingIntegration - #21385
Conversation
Uh oh!
There was an error while loading. Please reload this page.
size-limit report 📦
|
Lms24
left a comment
There was a problem hiding this comment.
Thanks for adding all of the tests! This makes it easier to understand and cover the behaviour.
Just had some remarks/questions but nothing that blocks this PR.
| userId: 123, | ||
| 'sentry.origin': 'auto.log.console', | ||
| 'sentry.message.parameter.0': 42, | ||
| 'sentry.message.parameter.1': { extra: 'data' }, |
There was a problem hiding this comment.
l: just to confirm, we stringify this attribute later, right?
There was a problem hiding this comment.
Yes, those are serialized in _INTERNAL_captureLog
| expect(formatConsoleArgs).toHaveBeenCalled(); | ||
| const call = vi.mocked(_INTERNAL_captureLog).mock.calls[0]![0]; | ||
| expect(call.message).toBe('[1,2,3] Array data'); | ||
| expect(call.attributes).not.toHaveProperty('0'); |
There was a problem hiding this comment.
q: why do we assert on the '0' key? Is this something we'd expect in an error case but don't want?
There was a problem hiding this comment.
In this case the log is console.info([1, 2, 3], 'Array Data') and we want to make sure that arrays are not treated as objects (on the first param). So by checking there is no 0, it means it took the fallback path and it did not attach the object keys as attributes.
| it('falls back when first arg is an Error', () => { | ||
| const err = new Error('Test error'); | ||
| triggerConsole('error', err, 'context info'); | ||
| const call = vi.mocked(_INTERNAL_captureLog).mock.calls[0]![0]; | ||
| expect(call.message).toContain('Error: Test error'); | ||
| expect(call.message).toContain('context info'); | ||
| expect(call.attributes).not.toHaveProperty('message'); | ||
| expect(call.attributes).not.toHaveProperty('name'); | ||
| }); | ||
| it('normalizes Error objects into message parameters', () => { | ||
| const err = new Error('connection refused'); | ||
| triggerConsole('error', 'Error occurred:', err); | ||
| const call = vi.mocked(_INTERNAL_captureLog).mock.calls[0]![0]; | ||
| expect(call.attributes?.['sentry.message.parameter.0']).toMatchObject({ | ||
| message: 'connection refused', | ||
| name: 'Error', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
hmm from a user perspective, I wouldn't understand why in the first case, my error is serialized to string and in the second case, it's extracted as an attribute. Don't wanna block this PR on it since I believe we settled on the extraction behaviour as you described in the PR description. But pointing this out as a potential discussion item to follow up on. I might also be missing something.
There was a problem hiding this comment.
Talked about this offline. This is basically the same behavior as it previously was. It might make sense on first sight to treat errors as objects but it opens a can of worms if we think about how to treat error properties as log attributes. It's better if we only care for plain objects and extract only those keys as attributes.
Lms24
left a comment
There was a problem hiding this comment.
Thx for clarifying my questions, LGTM!
Uh oh!
There was an error while loading. Please reload this page.
Aligns the console integration so object-first logs are structured, matching the consola integration behavior. If the first argument is an object, the keys are added as log attributes.
Reference PR (consola): #19534
The universal principle is this:
Example Snippet
Before
After