-
-
Notifications
You must be signed in to change notification settings - Fork 225
fix: Ensure structured logs from an SDK integration has sentry.origin #4566
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?
fix: Ensure structured logs from an SDK integration has sentry.origin #4566
Conversation
@sentry review |
bugbot review |
This comment has been minimized.
This comment has been minimized.
🚨 Bugbot couldn't runSomething went wrong. Try again by commenting "Cursor review" or "bugbot run", or contact support (requestId: serverGenReqId_8ec61e90-31f9-40d2-9718-56e6535d466d). |
bugbot review |
@sentry review |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4566 +/- ##
=======================================
Coverage 73.46% 73.47%
=======================================
Files 482 482
Lines 17678 17682 +4
Branches 3493 3493
=======================================
+ Hits 12988 12992 +4
Misses 3800 3800
Partials 890 890 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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'm not 100 % convinced of the origin names that we use right now, but also don't have a concrete suggestion in store ... if you don't mind, I'll let my mind wander a bit and will get back to you soon.
Also, feedback from other reviewers appreciated. @jamescrosswell what do you think?
From memory, we started setting the
|
Whoops, forgot to mention that we will be consolidating the category for |
The Structured Logs spec was updated. See #4618 for more detail: |
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 aligning the value of "sentry.origin".
One final feedback to fix the CHANGELOG.md
.
Then we could release this in 5.16.1
.
CHANGELOG.md
Outdated
- Upload linked PDBs to fix non-IL-stripped symbolication for iOS ([#4527](https://github.com/getsentry/sentry-dotnet/pull/4527)) | ||
- In MAUI Android apps, generate and inject UUID to APK and upload ProGuard mapping to Sentry with the UUID ([#4532](https://github.com/getsentry/sentry-dotnet/pull/4532)) | ||
- Fixed WASM0001 warning when building Blazor WebAssembly projects ([#4519](https://github.com/getsentry/sentry-dotnet/pull/4519)) | ||
- Structured Logs now have a `sentry.origin` attribute to so it's clearer where these come from ([#4566](https://github.com/getsentry/sentry-dotnet/pull/4566)) |
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.
thought: target branch version6
rather than main
?
We now merge new PRs towards the version6
branch, which will be released in 6.0.0
(and pre-releases), unless it's a critical bug fix.
But ... on the other hand ... this is important for Sentry internally ... so I believe I am convincing myself to actually do merge this against main
and release in 5.16.1
soon.
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 wouldn't say this is urgent, at most this is a "nice to have", because it doesn't really affect the functionality whole lot.
Not only that, it will also take time for the other SDK maintainers to follow the new spec for sentry.origin
and update theirs.
Thoughts? @jamescrosswell
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.
It may be quite a while before the majority of SDK users move to version 6.0.0 so probably worth leaving this in main and making a 5.16.1 release.
It'll get merged into version6 from there (along with a bunch of other stuff).
Co-authored-by: James Crosswell <[email protected]>
Co-authored-by: James Crosswell <[email protected]>
163dff3
to
6608542
Compare
Fixes #4560, fixes #4618
sentry.origin
attribute attached #4560sentry.origin
for Structured Logs according to the documentation #4618Problem
According to our SDK specification, we have to set
sentry.origin
whenever a log is sent from an integration.Solution
sentry-dotnet
has a few integrations that send out logs. This PR affects the following integrations:Sentry.Extensions.Logging
Sentry.Serilog
And affects other integrations that references
Sentry.Extensions.Logging
, or references it transitively.Comparison
Before, logs from

Sentry.Serilog
didn't havesentry.origin
:After, logs from

Sentry.Serilog
hassentry.origin
populated: