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

Diagnostic Visitors #1156

Merged
merged 5 commits into from
Jan 24, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ namespace Microsoft.Azure.Cosmos
using System.IO;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Azure.Cosmos.Diagnostics;
using Microsoft.Azure.Cosmos.Routing;
using Microsoft.Azure.Documents;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
// ------------------------------------------------------------
// Copyright (c) Microsoft Corporation. All rights reserved.
// ------------------------------------------------------------

namespace Microsoft.Azure.Cosmos.Diagnostics
{
using Microsoft.Azure.Cosmos.Query.Core.Metrics;

/// <summary>
/// Extracts the aggregated <see cref="BackendMetrics"/> from a <see cref="CosmosDiagnostics"/>.
/// </summary>
internal sealed class BackendMetricsExtractor : CosmosDiagnosticsInternalVisitor<(bool, BackendMetrics)>
Copy link
Contributor

@sboshra sboshra Jan 23, 2020

Choose a reason for hiding this comment

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

(bool, BackendMetrics [](start = 85, length = 21)

Nullable #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went for failure reason instead.


In reply to: 369908025 [](ancestors = 369908025)

Copy link
Contributor

@sboshra sboshra Jan 23, 2020

Choose a reason for hiding this comment

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

bool [](start = 86, length = 4)

enum for the failure reason #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call :)


In reply to: 369908749 [](ancestors = 369908749)

{
public static readonly BackendMetricsExtractor Singleton = new BackendMetricsExtractor();

private BackendMetricsExtractor()
{
// Private default constructor.
}

public override (bool, BackendMetrics) Visit(PointOperationStatistics pointOperationStatistics)
{
return (false, default);
}

public override (bool, BackendMetrics) Visit(CosmosDiagnosticsContext cosmosDiagnosticsContext)
{
return cosmosDiagnosticsContext.ContextList.Accept(this);
}

public override (bool, BackendMetrics) Visit(CosmosDiagnosticScope cosmosDiagnosticScope)
{
return (false, default);
}

public override (bool, BackendMetrics) Visit(CosmosDiagnosticsContextList cosmosDiagnosticsContextList)
{
BackendMetrics.Accumulator accumulator = default;
foreach (CosmosDiagnosticsInternal cosmosDiagnostics in cosmosDiagnosticsContextList)
{
(bool gotBackendMetric, BackendMetrics backendMetrics) = cosmosDiagnostics.Accept(this);
if (gotBackendMetric)
{
accumulator = accumulator.Accumulate(backendMetrics);
}
}

return (true, BackendMetrics.Accumulator.ToBackendMetrics(accumulator));
}

public override (bool, BackendMetrics) Visit(QueryPageDiagnostics queryPageDiagnostics)
{
if (!BackendMetrics.TryParseFromDelimitedString(queryPageDiagnostics.QueryMetricText, out BackendMetrics backendMetrics))
{
return (false, default);
}

return (true, backendMetrics);
}
}
}
52 changes: 24 additions & 28 deletions Microsoft.Azure.Cosmos/src/Diagnostics/CosmosDiagnosticScope.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
//------------------------------------------------------------

namespace Microsoft.Azure.Cosmos
namespace Microsoft.Azure.Cosmos.Diagnostics
{
using System;
using System.Collections.Generic;
Expand All @@ -15,28 +15,34 @@ namespace Microsoft.Azure.Cosmos
/// A scope is a section of code that is important to track.
/// For example there is a scope for serialization, retry handlers, etc..
/// </summary>
internal class CosmosDiagnosticScope : CosmosDiagnosticWriter, IDisposable
internal sealed class CosmosDiagnosticScope : CosmosDiagnosticsInternal, IDisposable
{
private readonly string Id;

private readonly Stopwatch ElapsedTimeStopWatch;

private readonly Action<TimeSpan> ElapsedTimeCallback;

internal TimeSpan? ElapsedTime { get; private set; }

private bool isDisposed = false;
internal CosmosDiagnosticScope(

public CosmosDiagnosticScope(
string name,
Action<TimeSpan> elapsedTimeCallback = null)
{
this.Id = name;
this.ElapsedTimeStopWatch = Stopwatch.StartNew();
this.ElapsedTime = null;
this.ElapsedTimeCallback = elapsedTimeCallback;
}

public string Id { get; }

public bool TryGetElapsedTime(out TimeSpan elapsedTime)
{
if (!this.isDisposed)
{
return false;
}
Copy link
Contributor

@sboshra sboshra Jan 23, 2020

Choose a reason for hiding this comment

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

I think we should not have a TryGet method here; rather, we should return whatever value the stopwatch has #Resolved

Copy link
Contributor

@j82w j82w Jan 23, 2020

Choose a reason for hiding this comment

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

It's better to have a TryGet. It's misleading if it returns the current elapsed time if the operation was never disposed of or still hasn't completed if there is a background task then it causes it to be ambiguous. #Resolved


elapsedTime = this.ElapsedTimeStopWatch.Elapsed;
return true;
}

public void Dispose()
{
if (this.isDisposed)
Expand All @@ -45,28 +51,18 @@ public void Dispose()
}

this.ElapsedTimeStopWatch.Stop();
this.ElapsedTime = this.ElapsedTimeStopWatch.Elapsed;
this.ElapsedTimeCallback?.Invoke(this.ElapsedTime.Value);
this.ElapsedTimeCallback?.Invoke(this.ElapsedTimeStopWatch.Elapsed);
this.isDisposed = true;
}

internal override void WriteJsonObject(JsonWriter jsonWriter)
public override void Accept(CosmosDiagnosticsInternalVisitor cosmosDiagnosticsInternalVisitor)
{
jsonWriter.WriteStartObject();
jsonWriter.WritePropertyName("Id");
jsonWriter.WriteValue(this.Id);
jsonWriter.WritePropertyName("ElapsedTime");

if (this.ElapsedTime.HasValue)
{
jsonWriter.WriteValue(this.ElapsedTime.Value);
}
else
{
jsonWriter.WriteValue("Timer Never Stopped.");
}
cosmosDiagnosticsInternalVisitor.Visit(this);
}

jsonWriter.WriteEndObject();
public override TResult Accept<TResult>(CosmosDiagnosticsInternalVisitor<TResult> visitor)
{
return visitor.Visit(this);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,7 @@
namespace Microsoft.Azure.Cosmos
{
using System;
using System.Collections.Generic;
using System.Globalization;
using System.Text;
using Newtonsoft.Json;

internal sealed class CosmosDiagnosticSummary
Expand Down Expand Up @@ -93,7 +91,6 @@ internal void Append(CosmosDiagnosticSummary newSummary)

internal void WriteJsonProperty(JsonWriter writer)
{
writer.WritePropertyName("Summary");
writer.WriteStartObject();
writer.WritePropertyName("StartUtc");
writer.WriteValue(this.StartUtc.ToString("o", CultureInfo.InvariantCulture));
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ namespace Microsoft.Azure.Cosmos
using System;
using System.IO;
using System.Text;
using Microsoft.Azure.Cosmos.Diagnostics;
using Newtonsoft.Json;

/// <summary>
Expand All @@ -14,24 +15,27 @@ namespace Microsoft.Azure.Cosmos
/// through the pipeline appending information as it goes into a list
/// where it is lazily converted to a JSON string.
/// </summary>
internal sealed class CosmosDiagnosticsContext : CosmosDiagnostics
internal sealed class CosmosDiagnosticsContext : CosmosDiagnosticsInternal
{
// Summary contains the high level overview of operations
// like start time, retries, and other aggregated information
internal CosmosDiagnosticSummary Summary { get; }
private bool isOverallScopeSet;

// Context list is detailed view of all the operations
internal CosmosDiagnosticsContextList ContextList { get; }

private bool isOverallScopeSet = false;

internal CosmosDiagnosticsContext()
public CosmosDiagnosticsContext()
{
this.Summary = new CosmosDiagnosticSummary(DateTime.UtcNow);
this.ContextList = new CosmosDiagnosticsContextList();
}

internal CosmosDiagnosticScope CreateOverallScope(string name)
/// <summary>
/// Contains the high level overview of operations like start time, retries, and other aggregated information
/// </summary>
public CosmosDiagnosticSummary Summary { get; }

/// <summary>
/// Detailed view of all the operations.
/// </summary>
public CosmosDiagnosticsContextList ContextList { get; }

public CosmosDiagnosticScope CreateOverallScope(string name)
{
CosmosDiagnosticScope scope;
// If overall is already set then let the original set the elapsed time.
Expand All @@ -57,26 +61,7 @@ internal CosmosDiagnosticScope CreateScope(string name)
return scope;
}

public override string ToString()
{
StringBuilder sb = new StringBuilder();
StringWriter sw = new StringWriter(sb);

using (JsonWriter jsonWriter = new JsonTextWriter(sw))
{
jsonWriter.WriteStartObject();
this.Summary.WriteJsonProperty(jsonWriter);
jsonWriter.WritePropertyName("Context");
jsonWriter.WriteStartArray();
this.ContextList.WriteJsonObject(jsonWriter);
jsonWriter.WriteEndArray();
jsonWriter.WriteEndObject();
}

return sb.ToString();
}

internal void AddContextWriter(CosmosDiagnosticWriter diagnosticWriter)
internal void AddContextWriter(CosmosDiagnosticsInternal diagnosticWriter)
Copy link
Contributor

@j82w j82w Jan 23, 2020

Choose a reason for hiding this comment

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

AddDiagnosticsInternal ? #Resolved

{
this.ContextList.AddWriter(diagnosticWriter);
}
Expand All @@ -92,5 +77,15 @@ internal void Append(CosmosDiagnosticsContext newContext)

this.ContextList.Append(newContext.ContextList);
}

public override void Accept(CosmosDiagnosticsInternalVisitor cosmosDiagnosticsInternalVisitor)
{
cosmosDiagnosticsInternalVisitor.Visit(this);
}

public override TResult Accept<TResult>(CosmosDiagnosticsInternalVisitor<TResult> visitor)
{
return visitor.Visit(this);
}
}
}
Original file line number Diff line number Diff line change
@@ -1,37 +1,54 @@
//------------------------------------------------------------
// Copyright (c) Microsoft Corporation. All rights reserved.
//------------------------------------------------------------
namespace Microsoft.Azure.Cosmos
namespace Microsoft.Azure.Cosmos.Diagnostics
{
using System;
using System.Collections;
using System.Collections.Generic;
using System.Text;
using Newtonsoft.Json;

internal sealed class CosmosDiagnosticsContextList : CosmosDiagnosticWriter
internal sealed class CosmosDiagnosticsContextList : CosmosDiagnosticsInternal, IEnumerable<CosmosDiagnosticsInternal>
{
private List<CosmosDiagnosticWriter> contextList { get; }
private List<CosmosDiagnosticsInternal> contextList { get; }

internal CosmosDiagnosticsContextList()
public CosmosDiagnosticsContextList(List<CosmosDiagnosticsInternal> contextList)
{
this.contextList = new List<CosmosDiagnosticWriter>();
this.contextList = contextList ?? throw new ArgumentNullException(nameof(contextList));
}

internal void AddWriter(CosmosDiagnosticWriter diagnosticWriter)
public CosmosDiagnosticsContextList()
: this(new List<CosmosDiagnosticsInternal>())
{
}

public void AddWriter(CosmosDiagnosticsInternal diagnosticWriter)
{
this.contextList.Add(diagnosticWriter);
}

internal void Append(CosmosDiagnosticsContextList newContext)
public void Append(CosmosDiagnosticsContextList newContext)
{
this.contextList.Add(newContext);
}

internal override void WriteJsonObject(JsonWriter jsonWriter)
public override void Accept(CosmosDiagnosticsInternalVisitor cosmosDiagnosticsInternalVisitor)
{
cosmosDiagnosticsInternalVisitor.Visit(this);
}

public override TResult Accept<TResult>(CosmosDiagnosticsInternalVisitor<TResult> visitor)
{
return visitor.Visit(this);
}

public IEnumerator<CosmosDiagnosticsInternal> GetEnumerator()
{
return this.contextList.GetEnumerator();
}

IEnumerator IEnumerable.GetEnumerator()
{
foreach (CosmosDiagnosticWriter writer in this.contextList)
{
writer.WriteJsonObject(jsonWriter);
}
return this.contextList.GetEnumerator();
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
//------------------------------------------------------------
// Copyright (c) Microsoft Corporation. All rights reserved.
//------------------------------------------------------------

namespace Microsoft.Azure.Cosmos.Diagnostics
{
using System.IO;

/// <summary>
/// Extends <see cref="CosmosDiagnostics"/> to expose internal APIs.
/// </summary>
internal abstract class CosmosDiagnosticsInternal : CosmosDiagnostics
bchong95 marked this conversation as resolved.
Show resolved Hide resolved
{
public abstract void Accept(CosmosDiagnosticsInternalVisitor visitor);

public abstract TResult Accept<TResult>(CosmosDiagnosticsInternalVisitor<TResult> visitor);

public override string ToString()
{
StringWriter stringWriter = new StringWriter();
this.WriteTo(stringWriter);
return stringWriter.ToString();
}

public void WriteTo(TextWriter textWriter)
{
CosmosDiagnosticsSerializerVisitor cosmosDiagnosticsSerializerVisitor = new CosmosDiagnosticsSerializerVisitor(textWriter);
this.Accept(cosmosDiagnosticsSerializerVisitor);
}
Copy link
Contributor

@sboshra sboshra Jan 23, 2020

Choose a reason for hiding this comment

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

consider moving this to the base class #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The base class doesn't have the visitor methods that are needed for this


In reply to: 369911329 [](ancestors = 369911329)

}
}
Loading