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.

Sign up for GitHub

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

Jump to bottom

CosmosDiagnostics: Fixes IndexOutOfRangeException #2099

Merged
merged 3 commits into from
Jan 4, 2021
Merged

CosmosDiagnostics: Fixes IndexOutOfRangeException #2099

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6ecb151
Fix diagnostics item count exception
Dec 29, 2020
4bc96e9
Revert test changes
Dec 29, 2020
3120f57
Merge branch 'master' into users/jawilley/diagnostics/fix_count
j82w Jan 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Microsoft.Azure.Cosmos/src/Diagnostics/CosmosDiagnosticsContextCore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ internal override void AddDiagnosticsInternal(CosmosDiagnosticsContext newContex
{
this.AddSummaryInfo(newContext);

this.ContextList.AddRange(newContext);
this.ContextList.Add(newContext);
}

public override void Accept(CosmosDiagnosticsInternalVisitor cosmosDiagnosticsInternalVisitor)
Expand Down
14 changes: 14 additions & 0 deletions Microsoft.Azure.Cosmos/src/Diagnostics/CosmosDiagnosticsSerializerVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ internal sealed class CosmosDiagnosticsSerializerVisitor : CosmosDiagnosticsInte
{
private const string DiagnosticsVersion = "2";
private readonly JsonWriter jsonWriter;
private bool rootDiagnosticContextVisited = false;

public CosmosDiagnosticsSerializerVisitor(TextWriter textWriter)
{
Expand Down Expand Up @@ -63,6 +64,19 @@ public override void Visit(PointOperationStatistics pointOperationStatistics)

public override void Visit(CosmosDiagnosticsContext cosmosDiagnosticsContext)
{
// Nested diagnostics should not include the summary
if (this.rootDiagnosticContextVisited)
{
foreach (CosmosDiagnosticsInternal cosmosDiagnosticsInternal in cosmosDiagnosticsContext)
{
cosmosDiagnosticsInternal.Accept(this);
}

return;
}

this.rootDiagnosticContextVisited = true;

this.jsonWriter.WriteStartObject();

this.jsonWriter.WritePropertyName("DiagnosticVersion");
Expand Down
1 change: 1 addition & 0 deletions Microsoft.Azure.Cosmos/src/Query/v3Query/CosmosQueryContextCore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ internal CosmosDiagnosticsContext GetAndResetDiagnostics()
{
CosmosDiagnosticsContext current = this.diagnosticsContext;
this.diagnosticsContext = CosmosDiagnosticsContext.Create(new RequestOptions());
current.GetOverallScope().Dispose();
return current;
}
}
Expand Down
15 changes: 8 additions & 7 deletions Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/CosmosDiagnosticsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ public async Task TestInitialize()
this.containerSettings = new ContainerProperties(id: Guid.NewGuid().ToString(), partitionKeyPath: PartitionKey);
ContainerResponse response = await this.database.CreateContainerAsync(
this.containerSettings,
throughput: 20000,
Copy link
Member

Choose a reason for hiding this comment

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

Does this change make any difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes it where there is multiple logical partitions which should cause the race condition to be exposed during our testing runs.

Copy link
Member

Choose a reason for hiding this comment

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

Will it impact the test durations?

Copy link
Contributor Author

@j82w j82w Jan 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

There was no noticeable difference when testing locally or in gates. It still drains the same number of items. Worst case is for some queries it might result in a extra read operation.

cancellationToken: this.cancellationToken);
Assert.IsNotNull(response);
Assert.IsNotNull(response.Container);
Expand Down Expand Up @@ -510,7 +511,7 @@ public async Task BulkOperationDiagnostic()
[TestMethod]
public async Task GatewayQueryPlanDiagnostic()
{
int totalItems = 3;
int totalItems = 10;
j82w marked this conversation as resolved.
Show resolved Hide resolved
IList<ToDoActivity> itemList = await ToDoActivity.CreateRandomItems(
this.Container,
pkCount: totalItems,
Expand Down Expand Up @@ -552,7 +553,7 @@ public async Task GatewayQueryPlanDiagnostic()
}

[TestMethod]
[DataRow(true)]
// [DataRow(true)]
j82w marked this conversation as resolved.
Show resolved Hide resolved
[DataRow(false)]
public async Task QueryOperationDiagnostic(bool disableDiagnostics)
{
Expand All @@ -563,12 +564,12 @@ public async Task QueryOperationDiagnostic(bool disableDiagnostics)
perPKItemCount: 1,
randomPartitionKey: true);

long readFeedTotalOutputDocumentCount = await this.ExecuteQueryAndReturnOutputDocumentCount(
queryText: null,
expectedItemCount: totalItems,
disableDiagnostics: disableDiagnostics);
//long readFeedTotalOutputDocumentCount = await this.ExecuteQueryAndReturnOutputDocumentCount(
// queryText: null,
// expectedItemCount: totalItems,
// disableDiagnostics: disableDiagnostics);

Assert.AreEqual(totalItems, readFeedTotalOutputDocumentCount);
//Assert.AreEqual(totalItems, readFeedTotalOutputDocumentCount);

//Checking query metrics on typed query
long totalOutputDocumentCount = await this.ExecuteQueryAndReturnOutputDocumentCount(
Expand Down
11 changes: 8 additions & 3 deletions ...oft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/Utils/DiagnosticValidators.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,12 +379,17 @@ public override void Visit(PointOperationStatistics pointOperationStatistics)

public override void Visit(CosmosDiagnosticsContext cosmosDiagnosticsContext)
{
Assert.IsFalse(this.isContextVisited, "Point operations should only have a single context");
this.isContextVisited = true;
this.StartTimeUtc = cosmosDiagnosticsContext.StartUtc;
this.TotalElapsedTime = cosmosDiagnosticsContext.GetRunningElapsedTime();

DiagnosticValidator.ValidateCosmosDiagnosticsContext(cosmosDiagnosticsContext);
// Only the first context needs to be validated. Inner contexts that were appended only
// require the content to validated.
if (!this.isContextVisited)
{
DiagnosticValidator.ValidateCosmosDiagnosticsContext(cosmosDiagnosticsContext);
}

this.isContextVisited = true;

foreach (CosmosDiagnosticsInternal diagnosticsInternal in cosmosDiagnosticsContext)
{
Expand Down
24 changes: 24 additions & 0 deletions Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Tests/CosmosDiagnosticsUnitTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
namespace Microsoft.Azure.Cosmos.Tests
{
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Net;
using System.Net.Http;
Expand Down Expand Up @@ -236,7 +237,30 @@ public void ValidateDiagnosticsAppendContext()
Thread.Sleep(TimeSpan.FromMilliseconds(100));
}

bool insertIntoDiagnostics1 = true;
bool isInsertDiagnostics = false;
// Start a background thread and ensure that no exception occurs even if items are getting added to the context
// when 2 contexts are appended.
Task.Run(() =>
{
isInsertDiagnostics = true;
CosmosSystemInfo cosmosSystemInfo = new CosmosSystemInfo(
cpuLoadHistory: new Documents.Rntbd.CpuLoadHistory(new List<Documents.Rntbd.CpuLoad>().AsReadOnly(), TimeSpan.FromSeconds(1)));
while (insertIntoDiagnostics1)
{
cosmosDiagnostics.AddDiagnosticsInternal(cosmosSystemInfo);
}
});

while (!isInsertDiagnostics)
{
Task.Delay(TimeSpan.FromMilliseconds(10)).Wait();
}

cosmosDiagnostics2.AddDiagnosticsInternal(cosmosDiagnostics);

// Stop the background inserts
insertIntoDiagnostics1 = false;
}

string diagnostics = cosmosDiagnostics2.ToString();
Expand Down