Add TelemetrySource to RateLimiterRejectedException #2346
Conversation
peter-csala
force-pushed
the
add-telemetry-source-to-rate-limiter-rejected-exception
branch
from
92e3f2e
to
cd0b75c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2346 +/- ##
==========================================
+ Coverage 85.40% 85.43% +0.03%
==========================================
Files 313 313
Lines 7467 7485 +18
Branches 1125 1128 +3
==========================================
+ Hits 6377 6395 +18
Misses 745 745
Partials 345 345
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| /// Gets BlahBlahBlah. | ||
| /// </summary> | ||
| /// <value>dummy.</value> | ||
| public ResilienceTelemetrySource TelemetrySource { get; } // Fix visibility |
There was a problem hiding this comment.
If I add the following line to the Polly.Core.csproj:
<InternalsVisibleToProject Include="Polly.RateLimiting" />then I don't have to change the access modifier from internal to public.
But it causes lots of compile time error, like complaining about RateLimiterResilienceStrategy's ExecuteCore should be proctected internal not just proctected, etc..
Do you have any other idea than making this property public?
There was a problem hiding this comment.
As far as I can see, we're currently doing this:
- Exposing the source;
- Using the source to pass to an exception;
- Having the exception turn the context into a string;
- Expose the string on the exception.
If that is the case, would this be better?
- Add a method to the context to turn it into a string (i.e. have it own its own logic to do that so it's not far from the type and could misalign if we changed the context in the future and forgot we were stringifying it elsewhere);
- Use that method to expose that string representation on the telemetry;
- Pass that string to the exception;
- Expose that string on the exception.
The above avoids the visibility problem, and makes the source string come from Polly.Core as the source-of-truth, rather than making something visible solely for rate limiting to make its own decisions about.
There was a problem hiding this comment.
With this approach we end up with following error:
Type 'RateLimiterRejectedException' already defines a member called 'RateLimiterRejectedException' with the same parameter types
public RateLimiterRejectedException(string telemetrySource)
public RateLimiterRejectedException(string message)
public RateLimiterRejectedException(string message, TimeSpan retryAfter)
public RateLimiterRejectedException(string telemetrySource, TimeSpan retryAfter)
...There was a problem hiding this comment.
So let's add a constructor that doesn't cause that problem then 😄 - for example, by manually specifying an exception message new RateLimiterRejectedException("The source {blah} go boom", source).
I think it would be much neater to have to manually construct a message because we've already used .ctor(string) for an explicit exception than have to expose the source just to avoid that.
There was a problem hiding this comment.
Why not just expose this property?
The ResilienceStrategyTelemetry is already public. I would indeed suggest to make the constructor of ResilienceTelemetrySource public, there is no reason for us to hinder the testability.
peter-csala
changed the title
| /// <summary> | ||
| /// Gets the source of the strategy which has thrown the exception, if known. | ||
| /// </summary> | ||
| public string? TelemetrySource { get; } |
There was a problem hiding this comment.
Why not use telemetry source here? The strategy name is not enough to uniquely identify the strategy.
There was a problem hiding this comment.
Thanks, added a comment :)
Pull Request
The issue or feature being addressed
#2345
Details on the issue fix or feature implementation
Done
TelemetrySourceproperty to theRateLimiterRejectedExceptionSourcebecause that would shadow the inheritedSourcepropertyConfirm the following