-
-
Notifications
You must be signed in to change notification settings - Fork 223
Ensure template is not sent for Structured logs with no parameters #4544
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?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4544 +/- ##
==========================================
+ Coverage 73.33% 73.48% +0.14%
==========================================
Files 479 482 +3
Lines 17509 17679 +170
Branches 3445 3495 +50
==========================================
+ Hits 12840 12991 +151
- Misses 3789 3798 +9
- Partials 880 890 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
if (Template is not null) | ||
// the SDK MUST NOT attach a sentry.message.template attribute if there are no parameters | ||
// https://develop.sentry.dev/sdk/telemetry/logs/#default-attributes | ||
if (Template is not null && Parameters is { Length: > 0 }) |
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.
potential issue: Accessing Length
could throw NullReferenceException
when default
if (Template is not null && Parameters is { Length: > 0 }) | |
if (Template is not null && !Parameters.IsDefaultOrEmpty) |
Admittedly, this is a bit pedantic. We currently have no flow setting the Parameters
to default
, but we technically could make a (semi-)related change in the future and then run into an NRE along the line.
ImmutableArray<KeyValuePair<string, object>> parameters = default;
_ = parameters.Length; // System.NullReferenceException: Object reference not set to an instance of an object.
The ImmutableArray<T>.Length
property is accessing the underlying array directly, which is null
when the struct ImmutableArray<T>
is (explicitly/implicitly) default
.
ImmutableArray<T>.IsEmpty
has a similar behavior.
To be on the safe side, I suggest using ImmutableArray<T>.IsDefaultOrEmpty
which ensures that this struct
instance isn't default
by checking whether the underlying array is null
before accessing its Length
.
Context:
I decided to use ImmutableArray<T>
here, because we expose the SentryLog.Parameters
publicly and to render this collection immutable through e.g. the user-defined SetBeforeSendLog(Func<SentryLog, SentryLog?>)
delegate. It does have its usage quirks for the benefit of similar performance characteristics to using a mutable Array directly.
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 think this us guarded against. If the condition was Parameters is { }
then it would ensure Parameters isn't null... and so Parameters is { Length: > 0 }
ensures that it's not null and has more than one element right?
EDIT: Oh, I see... default != null
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.
Yeah ... ImmutableArray`1
is a value type. But the underlying array is a reference type. And the Length
property does not perform a null
check on the underlying array, which actually is null should the ImmutableArray`1
be initialized to default
.
Co-authored-by: Stefan Pölz <[email protected]>
Co-authored-by: Stefan Pölz <[email protected]>
if (Template is not null) | ||
// the SDK MUST NOT attach a sentry.message.template attribute if there are no parameters | ||
// https://develop.sentry.dev/sdk/telemetry/logs/#default-attributes | ||
if (Template is not null && Parameters is { Length: > 0 }) |
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: Default ImmutableArray Length Access Throws Exception
The condition Parameters is { Length: > 0 }
can throw an InvalidOperationException
if Parameters
is a default ImmutableArray<T>
. Accessing the Length
property on a default ImmutableArray
causes this exception, which is inconsistent with how ImmutableArray
is handled elsewhere.
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.
Partially correct.
The get_Length
property does indeed throw when Parameters
is a default ImmutableArray<T>
.
But it does not throw an InvalidOperationException
, but a NullReferenceException
!
Resolves #4480:
sentry.message.template
on logs ifsentry.message.parameter
values are also defined #4480Note
Skip emitting
sentry.message.template
in structured logs without parameters; adds coverage and updates changelog.src/Sentry/SentryLog.cs
to only writeattributes.sentry.message.template
whenTemplate
is set andParameters
has length > 0.WriteTo_NoParameters_NoTemplate
theory intest/Sentry.Tests/SentryLogTests.cs
to assert presence/absence ofsentry.message.template
based on parameters.CHANGELOG.md
under Fixes to note templates aren’t sent for logs without parameters.Written by Cursor Bugbot for commit faad68a. This will update automatically on new commits. Configure here.