[Question]: Customize telemetry event severity?

[wjrogers]

What are you wanting to achieve?

I am using Polly v8 RateLimiter strategies to pro-actively restrict my client's request rate. I noticed that the OnRateLimiterRejected event is logged with Error severity. I want rate limiting events to be logged with at most Information severity because I am imposing the limit on myself. Is there a way to customize event severity?

What code or approach do you have so far?

I poked around the source code and tried a few things like handling RateLimiterStrategyOptions.OnRejected or calling builder.ConfigureTelemetry(new TelemetryOptions()), but I could not find a way to modify the event severity. All the properties exposed through these APIs are read-only.

Additional context

No response

[martincostello]

As far as I'm aware, as you've indicated, there's no way to change the severities at this time. It seems a reasonable suggestion to be able to allow this to be customised though.

[martintmk]

Thinking we could introduce something like this:

public class TelemetryOptions
{
    // ... existing properties

    public IDictionary<string, ResilienceEventSeverity> EventSeverityOverrides { get; set; }
}

BY default, it would be empty. If specified one could override the built-in severity for events. Also, I think we can downgrade the OnRejected to Warning. Error seems too high indeed.

Usage:

services.Configure<TelemetryOptions>(options =>
{
    options.EventSeverityOverrides["OnRateLimiterRejected"] = ResilienceEventSeverity.Information;
}); 

[peter-csala]

☝🏻 seems a bit error-prone. Non-existing key overrides would result in KeyNotFoundException.

Introducing hierarchical constants might be a better option IMHO.

options.EventSeverityOverrides[TelemetryNames.RateLimiter.OnRejected] = ResilienceEventSeverity.Information;

[martincostello]

Constants or not, we wouldn't get exceptions because we'd use TryGetValue() not the indexer.

[martintmk]

Since this is for advanced scenarios, the user can put whatever strings they want into the dictionary. If the name does not match up with existing telemetry events, we won't do any overrides.

Alternatively, we can allow delegate to be used (faster API and more flexible):

public class TelemetryOptions
{
    // ... existing properties
    public Func<ResilienceEvent, ResilienceEventSeverity>? ResilienceEventSeverityProvider { get; set; }
}

[wjrogers]

The delegate would be more flexible because it could conditionally change the severity based on other properties of the event. The static dictionary would be okay, too, though. With either solution, it'd be great if it could be applied to specific pipelines (as opposed to globally).

[peter-csala]

Constants or not, we wouldn't get exceptions because we'd use TryGetValue() not the indexer.

If the EventSeverityOverrides is exposed as an IDictionary then you can not enforce the usage of the TryGetValue inside the user provided delegate. People could still use the indexer.

[martintmk]

If the EventSeverityOverrides is exposed as an IDictionary then you can not enforce the usage of the TryGetValue inside the user provided delegate. People could still use the indexer.

It's either delegate or dictionary. I am gravitating towards using the delegate to override the severity (defaults to null) due to greater flexibility. In our code we would just invoke the delegate (if provided) just before logging/metering and it would change the severity for that event.

With either solution, it'd be great if it could be applied to specific pipelines (as opposed to globally).

You can explicitly call ConfigureTelemetry and it will override the telemetry just for that pipeline. Note about cloning the default options.

services.AddResiliencePipeline("my-key", (builder, context) =>
{
    builder.AddConcurrencyLimiter(10);

    // Resolve pre-configured options
    var global = context.GetOptions<TelemetryOptions>();
    var copy = new TelemetryOptions(global);

    // Modify the copy
    ...

    // Apply telemetry just for this pipeline
    builder.ConfigureTelemetry(copy);
});

Alternatively, we can allow ConfigureTelemetry to accept multiple options and it would merge them together transparently (without doing the cloning stuff).

[github-actions]

This issue is stale because it has been open for 60 days with no activity. It will be automatically closed in 14 days if no further updates are made.

[wjrogers]

not stale

[github-actions]

This issue is stale because it has been open for 60 days with no activity. It will be automatically closed in 14 days if no further updates are made.

[wjrogers]

not stale

[martintmk]

@wjrogers This #2072 PR addresses this. Can you take a look?

[martincostello]

Resolved by #2072.