Skip to content
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

Preview TraceWriter: Fixes Argument Null Exception / Null Reference Exception #2246

Merged
merged 3 commits into from
Feb 23, 2021

Conversation

bchong95
Copy link
Contributor

Preview TraceWriter: Fixes NRE

Description

The trace datum classes don't have a strong contract with null. Values like endpoint are sometimes allowed to be null. In the old trace writer we would call on Newtonsoft's JsonWriter.WriteValue(value), which is tolerant of null values. Currently we use Cosmos.Json.JsonWriter.WriteStringValue() which does not handle null. The fix is to just handle null in the writer. We can not push the invariants up to the type, since we don't know what code in production might set a value to null and don't want to cause a NullArgumentException at runtime. As we see diagnostics with nulls we can investigate what caused it and hunt down the values.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow the required format: "[Internal] Category: (Adds|Fixes|Refactors) Description"

Examples:
Diagnostics: Adds GetElapsedClientLatency to CosmosDiagnostics
PartitionKey: Fixes null reference when using default(PartitionKey)
[v4] Client Encryption: Refactors code to external project
[Internal] Query: Adds code generator for CosmosNumbers for easy additions in the future.

@bchong95 bchong95 changed the title Preview TraceWriter: Fixes NRE Internal TraceWriter: Fixes NRE Feb 22, 2021
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow the required format: "[Internal] Category: (Adds|Fixes|Refactors) Description"

Examples:
Diagnostics: Adds GetElapsedClientLatency to CosmosDiagnostics
PartitionKey: Fixes null reference when using default(PartitionKey)
[v4] Client Encryption: Refactors code to external project
[Internal] Query: Adds code generator for CosmosNumbers for easy additions in the future.

@bchong95 bchong95 changed the title Internal TraceWriter: Fixes NRE Preview TraceWriter: Fixes Null Reference Exception Feb 22, 2021
@github-actions github-actions bot dismissed stale reviews from themself February 22, 2021 20:24

All good!

Copy link
Contributor

@sboshra sboshra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@bchong95 bchong95 changed the title Preview TraceWriter: Fixes Null Reference Exception Preview TraceWriter: Fixes Argument Null Exception Feb 22, 2021
@bchong95 bchong95 changed the title Preview TraceWriter: Fixes Argument Null Exception Preview TraceWriter: Fixes Argument Null Exception / Null Reference Exception Feb 22, 2021
@sboshra sboshra merged commit df040f3 into master Feb 23, 2021
@sboshra sboshra deleted the users/brchon/Tracing/FixNRE branch February 23, 2021 01:31
@@ -158,7 +158,7 @@ public void Visit(PointOperationStatisticsTraceDatum pointOperationStatisticsTra
this.jsonWriter.WriteStringValue("PointOperationStatistics");

this.jsonWriter.WriteFieldName("ActivityId");
this.jsonWriter.WriteStringValue(pointOperationStatisticsTraceDatum.ActivityId);
this.WriteStringValueOrNull(pointOperationStatisticsTraceDatum.ActivityId);

this.jsonWriter.WriteFieldName("ResponseTimeUtc");
this.jsonWriter.WriteStringValue(pointOperationStatisticsTraceDatum.ResponseTimeUtc.ToString("o", CultureInfo.InvariantCulture));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WriteStringValueOrNull

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is after the diff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants