-
-
Notifications
You must be signed in to change notification settings - Fork 357
Support for applying scope attributes to logs #5579
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: main
Are you sure you want to change the base?
Changes from all commits
93a5b5c
fbc00cf
236f37a
06cc302
3c834cf
c588f82
14ecf4f
844a347
bbdb7a5
c5d6d89
18c843e
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 |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| /* eslint-disable complexity */ | ||
| import type { Integration, Log } from '@sentry/core'; | ||
| import { debug } from '@sentry/core'; | ||
| import { debug, getCurrentScope, getGlobalScope, getIsolationScope } from '@sentry/core'; | ||
| import type { ReactNativeClient } from '../client'; | ||
| import { NATIVE } from '../wrapper'; | ||
|
|
||
|
|
@@ -33,7 +33,7 @@ let NativeCache: Record<string, unknown> | undefined = undefined; | |
| * | ||
| * @param logAttributes - The log attributes object to modify. | ||
| * @param key - The attribute key to set. | ||
| * @param value - The value to set (only sets if truthy and key not present). | ||
| * @param value - The value to set (only sets if not null/undefined and key not present). | ||
| * @param setEvenIfPresent - Whether to set the attribute if it is present. Defaults to true. | ||
| */ | ||
| function setLogAttribute( | ||
|
|
@@ -42,7 +42,7 @@ function setLogAttribute( | |
| value: unknown, | ||
| setEvenIfPresent = true, | ||
| ): void { | ||
| if (value && (!logAttributes[key] || setEvenIfPresent)) { | ||
| if (value != null && (!logAttributes[key] || setEvenIfPresent)) { | ||
| logAttributes[key] = value; | ||
| } | ||
| } | ||
|
|
@@ -79,6 +79,13 @@ function processLog(log: Log, client: ReactNativeClient): void { | |
| // Save log.attributes to a new variable | ||
| const logAttributes = log.attributes ?? {}; | ||
|
|
||
| // Apply scope attributes from all active scopes (global, isolation, and current) | ||
| // These are applied first so they can be overridden by more specific attributes | ||
| const scopeAttributes = collectScopeAttributes(); | ||
|
Collaborator
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. could we try using I am not sure why Sentry JS Opted to only load data from isolationScope and currentScope, might be worth checking why.
Contributor
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. That's a good suggestion and my first try as well but the thing is
Contributor
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. Regarding the global scope: they do use it, they just call
Collaborator
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. Oh True, I did a quick test on capacitor and I was able to import it from
Collaborator
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. EDIT: it should be be exported on @sentry/core |
||
| Object.keys(scopeAttributes).forEach((key: string) => { | ||
| setLogAttribute(logAttributes, key, scopeAttributes[key], false); | ||
| }); | ||
|
|
||
| // Use setLogAttribute with the variable instead of direct assignment | ||
| setLogAttribute(logAttributes, 'device.brand', NativeCache.brand); | ||
| setLogAttribute(logAttributes, 'device.model', NativeCache.model); | ||
|
|
@@ -93,3 +100,42 @@ function processLog(log: Log, client: ReactNativeClient): void { | |
| // Set log.attributes to the variable | ||
| log.attributes = logAttributes; | ||
| } | ||
|
|
||
| /** | ||
| * Extracts primitive attributes from a scope and merges them into the target object. | ||
| * Only string, number, and boolean attribute values are included. | ||
| * | ||
| * @param scope - The scope to extract attributes from | ||
| * @param target - The target object to merge attributes into | ||
| */ | ||
| function extractScopeAttributes( | ||
| scope: ReturnType<typeof getCurrentScope>, | ||
| target: Record<string, string | number | boolean>, | ||
| ): void { | ||
| if (scope && typeof scope.getScopeData === 'function') { | ||
| const scopeAttrs = scope?.getScopeData?.().attributes || {}; | ||
| for (const [key, value] of Object.entries(scopeAttrs)) { | ||
| if (typeof value === 'string' || typeof value === 'number' || typeof value === 'boolean') { | ||
| target[key] = value; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Collects attributes from all active scopes (global, isolation, and current). | ||
| * Only string, number, and boolean attribute values are supported. | ||
| * Attributes are merged in order of precedence: global < isolation < current. | ||
| * | ||
| * @returns A merged object containing all scope attributes. | ||
| */ | ||
| function collectScopeAttributes(): Record<string, string | number | boolean> { | ||
| const attributes: Record<string, string | number | boolean> = {}; | ||
|
|
||
| // Collect attributes from all scopes in order of precedence | ||
| extractScopeAttributes(getGlobalScope(), attributes); | ||
| extractScopeAttributes(getIsolationScope(), attributes); | ||
| extractScopeAttributes(getCurrentScope(), attributes); | ||
|
|
||
| return 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.
Bug: The check
!logAttributes[key]incorrectly uses truthiness instead of key presence, causing existing falsy attribute values (0,false,"") to be overridden when they shouldn't be.Severity: MEDIUM
Suggested Fix
Replace the truthy check
!logAttributes[key]with a proper key presence check, such as!(key in logAttributes). This will correctly verify if the key exists in thelogAttributesobject, regardless of its value's truthiness.Prompt for AI Agent
Did we get this right? 👍 / 👎 to inform future reviews.