-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(core): Apply scope attributes to logs #18184
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?
Conversation
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
52339f2 to
b97b3f4
Compare
a3e204d to
205752c
Compare
5053159 to
e5785ea
Compare
fbbd768 to
04d75b1
Compare
e5785ea to
561085c
Compare
e575427 to
5388d82
Compare
|
UPDATE: See PR description for updated behaviour OK, so we still have 2 problems and a minor inconvenience here:
|
|
To at least offer partial support, we settled on only serializing primitive, and discarding non-primitive scope attribute values. I update the PR and the PR description accordingly. |
| * @param value - The value of the log attribute. | ||
| * @returns The serialized log attribute. | ||
| */ | ||
| export function logAttributeToSerializedLogAttribute(value: unknown): SerializedLogAttributeValue { |
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.
Since we have a fallback parameter in attributeValueToTypedAttributeValue, we can get rid of this logic. For now the bundle size savings are minimal but once we migrate logs to the new unified function, it should pay off.
|
|
||
| mergeAndOverwriteScopeData(data, 'extra', extra); | ||
| mergeAndOverwriteScopeData(data, 'tags', tags); | ||
| mergeAndOverwriteScopeData(data, 'attributes', attributes); |
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 was still missing in #18165
| attributes: { | ||
| ...commonAttributes, | ||
| log_attr: { value: 'log_attr_1', type: '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.
Bug: Node integration test uses partial matching for attributes (Bugbot Rules)
Flagging per review rules: The test uses toMatchObject (via assertSentryLogContainer) which is a relaxed assertion. The test scenario sets array_attr: [1, 2, 3] on the global scope, which should NOT appear in logs (since non-primitive scope attributes are discarded). However, the test only verifies that expected attributes are present, not that array_attr is absent. If a regression caused non-primitive scope attributes to leak into logs, this test would not catch it. The browser integration test correctly uses toEqual for exact matching.
This PR adds support for scope attributes on logs. For now, only primitive attribute values are supported despite type declarations of
Scope.setAttribute(s)also allowing array attribute values. The reason for this limited support is that Relay still discards array attribute values. Therefore, our serialization strategy for now is:Usage Example
Some behavior notes:
closes #18159