Particular / NServiceBus.Metrics

NServiceBus.Metrics provides a convenient way of gathering metrics like processing time or critical time.
Other
5 stars 2 forks source link

Probes observer registration API does not have any context properties #40

Open ramonsmits opened 7 years ago

ramonsmits commented 7 years ago

I'm implementing an observer and notices that probes do not have any context except a name. For example, I don't know the message ID, the incoming message type, or other types. That is not needed at the global level but is when you would like to drill down:

Very nice to have context arguments!

Current observer registration, no context available for probe values:

foreach (var duration in context.Durations)
{
    duration.Register(durationLength =>
    {
        // No context of the duration
    });
}

Improved API for observer registrations, context available for probe values. A collector can capture any property it is interested in.

foreach (var duration in context.Durations)
{
    if (duration.Name == "Processing Time")
    {
        duration.Register(measurement =>
        {
             var context = (IIncomingLogicalMessageContext) measurement.Context;
             var headers = context.Headers;
             var messageName = context.Message.MessageType.Name;
             var messageIntent = headers[Headers.MessageIntent];

             // Now I can retrieve additional context for a specific
             // probe type and use that data for metrics.
        });
    }
}
Scooletz commented 7 years ago

@ramonsmits Let me describe how these values are reported now and follow it with further reasoning. Currently, occurrences and durations are sent using smart batching in a low overhead, byte-aware way. This means that they aim at being able to send a lot of data, fast, in small batches.

If we want to drill down, we can do it still using the current protocol. We can gather and send data per message type.

If the above is not enough, we'd need to pack more data into a single message, like: message type dictionary, but this becomes quite heavy. What are the probe start/stop? Are these, beside reporting per message type, driven by customers' questions? What are scenarios for this? The final: do these questions belong really in here?

Metrics reporting was built to provide a low overhead, fast reporting of simple measurements.

ramonsmits commented 7 years ago

@Scooletz It is impossible to generate all combinations of metrics in the stream. It is up to the reporter to decide on the location of the probe and the context what it would like to report. That way the reporter its implementation decides how much overhead is added.

The start/stop values are already created based on the Stopwatch. The context is also already there as it is part of the behavior and could be just an instance reference. We would never be able to copy specific context values as a customer could in theory even choose to include an incoming header value.

The collector receiving the probe is responsible to pick what it wants to report. If it wants to traverse the context for additional properties that would be just fine as this is happening async anyway not blocking the probe itself I assume. It is only the collector that knows if it wants to report average or current values or if it is global or context specific.

I do think they belong here otherwise the whole probe package is pretty worthless if you would like to retrieve such data as then you basically required to clone the metrics package and do exactly this.

Maybe the alternative is having a lean and mean probe extension as currently exists and a still lean

Your last sentence does not seem to be complete.

ramonsmits commented 7 years ago

@Scooletz As become clear in our discussion on zoom we had different components in mind. I've extended the issue description with an example of how context could be provided if you wish to add another custom telemetry solution.

Is any thing missing from the issue description?

Scooletz commented 7 years ago

Thank you @ramonsmits

The issue title and the description are now much better. I slightly adjusted the snippet registering a custom observer by: