-
Notifications
You must be signed in to change notification settings - Fork 273
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
[geneva] Nullable annotations for TLDExporter folder #2085
base: main
Are you sure you want to change the base?
[geneva] Nullable annotations for TLDExporter folder #2085
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2085 +/- ##
==========================================
- Coverage 73.91% 64.65% -9.27%
==========================================
Files 267 35 -232
Lines 9615 3596 -6019
==========================================
- Hits 7107 2325 -4782
+ Misses 2508 1271 -1237
Flags with carried forward coverage won't be shown. Click here to find out more.
|
} | ||
catch | ||
{ | ||
repr = $"ERROR: type {value.GetType().FullName} is not supported"; | ||
repr = $"ERROR: type {value!.GetType().FullName} is not supported"; |
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 can throw an exception if value = 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.
We could but we would have to pay for a null
check for every field serialized. I went and checked, seems like none of the upstream code actually will ever pass a null
(also added the assert to validate) so this at the end of the day is just being defensive. Even with a null
check users could do something silly like this...
activity.SetTag("MyTag", new MyType());
class MyType
{
public override string? ToString() => null;
}
...which would cause the Convert.ToString
call there to return null
even though value
itself was something real! Spooky 👻
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.
okay, if you checked the upstream code then I'm fine continuing as is. :)
var fullName = logRecord.Exception.GetType().FullName; | ||
if (!string.IsNullOrEmpty(fullName)) | ||
{ | ||
eb.AddCountedAnsiString("ext_ex_type", fullName, Encoding.UTF8); | ||
} | ||
|
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.
for my learning, when would the Exception Type be null or empty?
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.
Exception.Type
shouldn't be null
but Type.FullName
may be null. Why would FullName
be null
? Who knows! Probably some edge case thing not often seen like a Type
dynmaically constructed at runtime? This change here makes it defensive should we ever run into it 🤷
} | ||
|
||
return result; | ||
return ExportResult.Failure; |
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.
Looks like we may have been incorrectly returning Success
when the eventProvider was not enabled.
Does this need a bug fix line in the Changelog?
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 it was fine before just kind of awkwardly written. All I did here was switch it around to hopefully make it more readable.
if (entry.Key.StartsWith("otel.status_", StringComparison.Ordinal)) | ||
{ | ||
if (string.Equals(Convert.ToString(entry.Value, CultureInfo.InvariantCulture), "ERROR", StringComparison.Ordinal)) | ||
var keyPart = entry.Key.AsSpan().Slice(12); | ||
if (keyPart is "code") |
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 is cool. I had to do some reading but this should be a allocation-free way of working with substrings. learned something new today :)
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.
Spans are magic 🤣 But this isn't so much about avoiding allocations as it is just speeding up the logic. If you look at the before it checked if the key was "otel.status_code" and then it checked if the key was "otel.status_description". What I did was switch it to see if the key starts with "otel.status_" if it doesn't, we know it isn't going to match and we can skip further processing. Only if the key starts with "otel.status_" then do we check for "code" or "description" essentially so we don't do any repeat work.
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.
LGTM. I left some minor comments
Changes
Merge requirement checklist