App-vNext
Introduce `ExecutionRejectedException.TelemetrySource` property
Just an experiment on how we can uniformly introduce telemetry source to all our exceptions.
The idea is that rather to introduce many new overloads to exception constructors, we can update this property centrally. It's niche enough< so my thinking is we don't need to expose this parameter in exception constructor. If someone complains, we can do it later anyway.
Contributes to #2346 and #2345
cc @peter-csala, @martincostello
Your thoughts?
Pull Request
The issue or feature being addressed
Details on the issue fix or feature implementation
Confirm the following
- [ ] I started this PR by branching from the head of the default branch
- [ ] I have targeted the PR to merge into the default branch
- [ ] I have included unit tests for the issue/feature
- [ ] I have successfully run a local build
If we go with this approach to decorate all Polly exceptions (that derives from the ExecutionRejectedException) then that we would generate consistent behavior across the strategies. BUT for those exceptions that are not Polly exceptions we don't have this.
So, if we extend the Polly exceptions with TelemetrySource then IMHO we should also provide a workaround for non-Polly exceptions as well to be able to use the telemetry source regardless the thrown exception.
If we go with this approach to decorate all Polly exceptions (that derives from the
ExecutionRejectedException) then that we would generate consistent behavior across the strategies. BUT for those exceptions that are not Polly exceptions we don't have this.So, if we extend the Polly exceptions with
TelemetrySourcethen IMHO we should also provide a workaround for non-Polly exceptions as well to be able to use the telemetry source regardless the thrown exception.
If we go with this approach to decorate all Polly exceptions (that derives from the
ExecutionRejectedException) then that we would generate consistent behavior across the strategies. BUT for those exceptions that are not Polly exceptions we don't have this.So, if we extend the Polly exceptions with
TelemetrySourcethen IMHO we should also provide a workaround for non-Polly exceptions as well to be able to use the telemetry source regardless the thrown exception.
How about we treat the UpdateTelemetrySource as an pub-internal API (albeit hidden, so it can be used in Polly.RateLimiting). We will add the TelemetrySource property to all our exceptions.
If someone complains that they want to attach this property when constructing the exceptions, we can do the follow-up where we expose a new set of constructors for all relevant exceptions?
Alternatively, we can do it now. One thing I feel quite convinced about is to expose the full ResilienceTelemetrySource property on the exception, as having just string identifier is ambiguous and cannot identify the resilience strategy properly.
How about we treat the
UpdateTelemetrySourceas an pub-internal API (albeit hidden, so it can be used inPolly.RateLimiting). We will add theTelemetrySourceproperty to all our exceptions.If someone complains that they want to attach this property when constructing the exceptions, we can do the follow-up where we expose a new set of constructors for all relevant exceptions?
Alternatively, we can do it now. One thing I feel quite convinced about is to expose the full
ResilienceTelemetrySourceproperty on the exception, as having just string identifier is ambiguous and cannot identify the resilience strategy properly.
I'm okay with this approach. Do you want to finish this PR or shall I incorporate the idea into my PR?
How about we treat the
UpdateTelemetrySourceas an pub-internal API (albeit hidden, so it can be used inPolly.RateLimiting). We will add theTelemetrySourceproperty to all our exceptions. If someone complains that they want to attach this property when constructing the exceptions, we can do the follow-up where we expose a new set of constructors for all relevant exceptions? Alternatively, we can do it now. One thing I feel quite convinced about is to expose the fullResilienceTelemetrySourceproperty on the exception, as having just string identifier is ambiguous and cannot identify the resilience strategy properly.I'm okay with this approach. Do you want to finish this PR or shall I incorporate the idea into my PR?
If you are OK with incorporating this, please continue in your PR :)
@martincostello Are you fine with above approach?
I'm not against the idea of the pub-ternal API. I think if we go down that route it's better to do the minimal amount and do more later if asked for, otherwise it's going to end up being quite a big change in terms of additional API surface area.
I'm not against the idea of the pub-ternal API. I think if we go down that route it's better to do the minimal amount and do more later if asked for, otherwise it's going to end up being quite a big change in terms of additional API surface area.
this proposal really only adds one new pub API - i.e. ExecutionRejectedException.TelemetrySource so that works perfectly with your comment :)
@peter-csala please go ahead!
I'm not against the idea of the pub-ternal API. I think if we go down that route it's better to do the minimal amount and do more later if asked for, otherwise it's going to end up being quite a big change in terms of additional API surface area.
this proposal really only adds one new pub API - i.e.
ExecutionRejectedException.TelemetrySourceso that works perfectly with your comment :)@peter-csala please go ahead!
Unfortunately no :( The ExecutionRejectedException is used by the Polly V7 API as well... So, the old RateLimitRejectedException, and BulkheadRejectedException would be impacted as well.
I try to extend only the Polly.Core and Polly.RateLimiting exceptions.
I'm not against the idea of the pub-ternal API. I think if we go down that route it's better to do the minimal amount and do more later if asked for, otherwise it's going to end up being quite a big change in terms of additional API surface area.
this proposal really only adds one new pub API - i.e.
ExecutionRejectedException.TelemetrySourceso that works perfectly with your comment :) @peter-csala please go ahead!Unfortunately no :( The
ExecutionRejectedExceptionis used by the Polly V7 API as well... So, the oldRateLimitRejectedException, andBulkheadRejectedExceptionwould be impacted as well.I try to extend only the Polly.Core and Polly.RateLimiting exceptions.
I would say it's ok, it's nullable anyway so on V7 this property will never be filled.