-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat(logs): Expand attributes section for logs spec and specify defaults for every platform #13832
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
develop-docs/sdk/telemetry/logs.mdx
Outdated
| Beyond these attributes, we are exploring if the SDK should also send OS, user, and device information automatically (via reading the appropriate contexts from the scope). Given this behaviour can easily be added as a new feature to the SDK, it does not have to be part of the initial SDK implementation until we make a finalized decision. | ||
| #### User Attributes | ||
|
|
||
| If the log is associated with a user, the SDK should attach the following if available: |
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.
Please add that these user attributes should only be added if sendDefaultPii: true.
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.
Done with 769c92d
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 adding that. I asked a few questions on why we use a subset of available attributes. We can also follow up on these questions in other PRs. Apart from that, LGTM.
| 4. `sentry.sdk.name`: The name of the SDK that sent the log | ||
| 5. `sentry.sdk.version`: The version of the SDK that sent the log |
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.
Do we also plan to support integrations, features, and settings that already exist? These can be handy for investigating issues and tracking the adoption of integrations and features.
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.
At the current moment no. We want to avoid making the payload too large.
We can re-evaluate this based on internal and external feedback.
|
|
||
| 1. `user.id`: The user ID. Maps to `id` in the [User](/sdk/data-model/event-payloads/user/) payload. | ||
| 2. `user.name`: The username. Maps to `username` in the [User](/sdk/data-model/event-payloads/user/) payload. | ||
| 3. `user.email`: The email address. Maps to `email` in the [User](/sdk/data-model/event-payloads/user/) payload. |
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.
What about the other existing attributes such as ip_address, geo, city, country_code and region? If we deliberately decided against having these, it would be good to explain here why.
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.
Geo, city, country_code and region are set by Relay.
develop-docs/sdk/telemetry/logs.mdx
Outdated
|
|
||
| #### Mobile, Desktop, and Native SDKs | ||
|
|
||
| For mobile, desktop, and native SDKs (Android, iOS, Electron, etc.), the SDKs should attach the following: |
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.
Again why is this list incomplete? What drove us to only add a subset? It's OK, but we should point out why here.
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.
I added a note as part of 9fc6809 to address this.
Co-authored-by: Philipp Hofmann <[email protected]>
Co-authored-by: Philipp Hofmann <[email protected]>
Co-authored-by: Philipp Hofmann <[email protected]>
| "device.brand": "Apple", | ||
| "device.model": "iPhone 15 Pro Max", | ||
| "device.family": "iPhone" | ||
| } |
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.
@AbhiPrasad do we plan to normalize logs too? Asking because e.g. for Android we backfill device.name to a human-readable name here, so it becomes e.g. Galaxy Tab A7 instead of SM-T500 or something like that. Would be great to have that for logs eventually, I suppose.
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.
Great point - I created https://linear.app/getsentry/issue/LOGS-155/enrich-devicename-attribute-to-logs-in-relay to track this issue.
…lts for every platform (#13832) resolves https://linear.app/getsentry/issue/LOGS-28 After testing with some SDKs and getting feedback, we've decided on a list of default attributes the SDKs should try to attach to logs. This includes 1. user attributes from https://develop.sentry.dev/sdk/data-model/event-payloads/user/ 2. browser attributes, parsed from user agents (getsentry/relay#4757) 3. os and device attributes attached to mobile, desktop, and native sdks, based on https://develop.sentry.dev/sdk/data-model/event-payloads/contexts/ I also re-organized this section to make things a bit more clear. --------- Co-authored-by: Philipp Hofmann <[email protected]>
resolves https://linear.app/getsentry/issue/LOGS-28
After testing with some SDKs and getting feedback, we've decided on a list of default attributes the SDKs should try to attach to logs.
This includes
I also re-organized this section to make things a bit more clear.