[geneva] Nullable annotations for TLDExporter folder #2085
Conversation
github-actions
bot
assigned cijothomas, CodeBlanch, rajkumar-rangaraj, reyang and utpilla
github-actions
bot
requested review from
cijothomas,
rajkumar-rangaraj,
reyang and
utpilla
github-actions
bot
added
the
comp:exporter.geneva
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.
I think this can throw an exception if value = null!
There was a problem hiding this comment.
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.
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.
for my learning, when would the Exception Type be null or empty?
There was a problem hiding this comment.
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.
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.
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.
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.
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.
Changes
Merge requirement checklist