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

Introduce ExecutionRejectedException.TelemetrySource property #2352

Closed
wants to merge 1 commit into from

Conversation

martintmk
Copy link
Contributor

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

@martintmk martintmk changed the title Introduce ExectuionRejectedException.TelemetrySource property Introduce ExecutionRejectedException.TelemetrySource property Oct 21, 2024
/// Updates the <see cref="ExecutionRejectedException.TelemetrySource"/> property of the exception.
/// </summary>
/// <param name="exception">The exception to update.</param>
public void UpdateTelemetrySource(ExecutionRejectedException exception)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can even hide this initially with [EditorBrowsable(EditorBrowsableState.Never)] ?

{
Exception exception => new BrokenCircuitException(BrokenCircuitException.DefaultMessage, retryAfter, exception),
_ => new BrokenCircuitException(BrokenCircuitException.DefaultMessage, retryAfter)
};

_telemetry.UpdateTelemetrySource(ex);
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO this method call is super easy to forget. Also if someone wants to construct any of the Polly exception (for whatever reason) then she has to be aware of this feature and has to use it.

@peter-csala
Copy link
Contributor

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.

@martintmk
Copy link
Contributor Author

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 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.

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.

@peter-csala
Copy link
Contributor

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.

I'm okay with this approach. Do you want to finish this PR or shall I incorporate the idea into my PR?

@martintmk
Copy link
Contributor Author

martintmk commented Oct 21, 2024

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.

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?

@martincostello
Copy link
Member

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.

@martintmk
Copy link
Contributor Author

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!

@peter-csala
Copy link
Contributor

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!

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.

@martintmk
Copy link
Contributor Author

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!

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 would say it's ok, it's nullable anyway so on V7 this property will never be filled.

@martintmk
Copy link
Contributor Author

Duplicate of #2346

@martintmk martintmk marked this as a duplicate of #2346 Oct 22, 2024
@martintmk martintmk closed this Oct 22, 2024
@martincostello martincostello deleted the mtomka/exception-telemetry-source branch October 22, 2024 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants