-
Notifications
You must be signed in to change notification settings - Fork 647
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
Emit critical time and processing time meters #6953
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
namespace NServiceBus.AcceptanceTests.Core.OpenTelemetry; | ||
|
||
using System.Linq; | ||
using System.Threading.Tasks; | ||
using NServiceBus; | ||
using NServiceBus.AcceptanceTesting; | ||
using NUnit.Framework; | ||
|
||
public class When_processing_completes : OpenTelemetryAcceptanceTest | ||
{ | ||
[Test] | ||
public async Task Should_report_processing_and_critical_time() | ||
{ | ||
using var metricsListener = TestingMetricListener.SetupNServiceBusMetricsListener(); | ||
_ = await Scenario.Define<Context>() | ||
.WithEndpoint<MyEndpoint>(e => e | ||
.When(s => s.SendLocal(new MyMessage()))) | ||
.Done(c => c.HandlerInvoked) | ||
.Run(); | ||
|
||
var processingTime = metricsListener.GetReportedMeasurements<double>("nservicebus.messaging.processingtime").Single(); | ||
var criticalTime = metricsListener.GetReportedMeasurements<double>("nservicebus.messaging.criticaltime").Single(); | ||
|
||
Assert.Greater(processingTime.Value, 50.0); | ||
Assert.Greater(criticalTime.Value, processingTime.Value); | ||
} | ||
|
||
class Context : ScenarioContext | ||
{ | ||
public bool HandlerInvoked { get; set; } | ||
} | ||
|
||
class MyEndpoint : EndpointConfigurationBuilder | ||
{ | ||
public MyEndpoint() => EndpointSetup<OpenTelemetryEnabledEndpoint>(); | ||
|
||
class MyMessageHandler : IHandleMessages<MyMessage> | ||
{ | ||
Context textContext; | ||
|
||
public MyMessageHandler(Context textContext) => this.textContext = textContext; | ||
|
||
public async Task Handle(MyMessage message, IMessageHandlerContext context) | ||
{ | ||
await Task.Delay(50); | ||
|
||
textContext.HandlerInvoked = true; | ||
} | ||
} | ||
} | ||
|
||
public class MyMessage : IMessage | ||
{ | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
namespace NServiceBus; | ||
using System; | ||
|
||
static class Extensions | ||
{ | ||
public static bool TryGetTimeSent(this ReceivePipelineCompleted completed, out DateTimeOffset timeSent) | ||
{ | ||
var headers = completed.ProcessedMessage.Headers; | ||
if (headers.TryGetValue(Headers.TimeSent, out var timeSentString)) | ||
{ | ||
timeSent = DateTimeOffsetHelper.ToDateTimeOffset(timeSentString); | ||
return true; | ||
} | ||
timeSent = DateTimeOffset.MinValue; | ||
return false; | ||
} | ||
|
||
public static bool TryGetDeliverAt(this ReceivePipelineCompleted completed, out DateTimeOffset deliverAt) | ||
{ | ||
var headers = completed.ProcessedMessage.Headers; | ||
if (headers.TryGetValue(Headers.DeliverAt, out var deliverAtString)) | ||
{ | ||
deliverAt = DateTimeOffsetHelper.ToDateTimeOffset(deliverAtString); | ||
return true; | ||
} | ||
deliverAt = DateTimeOffset.MinValue; | ||
return false; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ class Meters | |
{ | ||
internal static readonly Meter NServiceBusMeter = new Meter( | ||
"NServiceBus.Core", | ||
"0.1.0"); | ||
"0.2.0"); | ||
|
||
internal static readonly Counter<long> TotalProcessedSuccessfully = | ||
NServiceBusMeter.CreateCounter<long>("nservicebus.messaging.successes", description: "Total number of messages processed successfully by the endpoint."); | ||
|
@@ -16,4 +16,10 @@ class Meters | |
|
||
internal static readonly Counter<long> TotalFailures = | ||
NServiceBusMeter.CreateCounter<long>("nservicebus.messaging.failures", description: "Total number of messages processed unsuccessfully by the endpoint."); | ||
|
||
internal static readonly Histogram<double> ProcessingTime = | ||
NServiceBusMeter.CreateHistogram<double>("nservicebus.messaging.processingtime", "ms", "The time in milliseconds between when the message was pulled from the queue until processed by the endpoint."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the relationship between NServiceBusMeter.CreateHistogram and OpenTelemetry? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See CreateHistogram: NserviceBusMeter is of type Meter: There is actually no direct reference to the OpenTelemetry SDK from Meter in .NET or from NServiceBus, even though there is an It was actually a great decision by the .NET team to keep system.diagnostics.metrics decoupled from OpenTelemetry, seeing as how many unified observability/telemetry tools have come before it. There's no guarantee OpenTelemetry will be the last, but there has been a great deal more buying across the industry it seems. That's all in the realm of interesting, but not important. What is important is that Meter surfaces information in a way it can be consumed by otel and any proprietary tools or future standard tools. Sorry about mistakes, while on my phone. |
||
|
||
internal static readonly Histogram<double> CriticalTime = | ||
NServiceBusMeter.CreateHistogram<double>("nservicebus.messaging.criticaltime", "ms", "The time in milliseconds between when the message was sent until processed by the endpoint."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any chance this will make it into the NSB v9 release that is RTM? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} |
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.
@bbrandt I think you're referring to this right? From what I understand is that they recommend to add a suffix to identify unit of measurement. I'm not sure if this value can be a double/float. We likely still want to report duration in floating point when reporting in seconds. The current code also passed double (result of
TotalMilliseconds
).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.
btw, for histograms using seconds likely wouldn't make much sense for message processing. Wouldn't you want to have millisecond granularity at the base of a histogram?
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 addressed in comments for open-telemetry/opentelemetry-dotnet#4797.
If you create a histogram with ms units, the default histogram boundaries to { 0, 5, 10, 25, 50, 75,
100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000} and if you create with seconds as the unit the default histogram boundaries are those same values, divided by 1000.
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.
Yes. Value will be reported as a double.
My preference is using https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.stopwatch?view=net-8.0 instead of DateTime.UtcNow, but this is a minor difference.