Add Retryable/Non-Retryable Exception + API Calls Metrics for O365#6202
Add Retryable/Non-Retryable Exception + API Calls Metrics for O365#6202zeligsonbrett wants to merge 0 commit into
Conversation
zeligsonbrett
requested review from
KarstenSchnitter,
chenqi0805,
dinujoh,
dlvenable,
engechas,
graytaylor0,
kkondaka,
oeyh,
san81,
sb2k16 and
srikanthjg
as code owners
san81
left a comment
san81
left a comment
There was a problem hiding this comment.
Add a few comments. Please take a look.
| List<Record<Event>> records = new ArrayList<>(); | ||
|
|
||
| do { | ||
| apiCallsCounter.increment(); |
There was a problem hiding this comment.
There is some logic inside the service.searchAuditLogs method that may prevent an api call to take place. Looks like this is not the right place for this counter.
| throw new Office365Exception("Missing contentUri in metadata", false); | ||
| } | ||
|
|
||
| apiCallsCounter.increment(); |
There was a problem hiding this comment.
There is already a counter auditLogsRequestedCounter that counts the number of auditLog calls made. This additional counter is to count all the api calls?
There was a problem hiding this comment.
Yes, this counter is for all API calls.
| nonRetryableErrorsCounter.increment(); | ||
| } | ||
| } else { | ||
| retryableErrorsCounter.increment(); |
There was a problem hiding this comment.
Are we sure about this? Like considering everything else as retryable. For example, I see that searchAuditLogs could through IllegalArgumentException which I suppose not retryable?
There was a problem hiding this comment.
Yea, this is a good point. I think we should assume non-retryable by default and only retry for known retryable.
Also related to this: we should combine this logic with the retry logic. If somebody changes that logic, these metrics may be missed.
| nonRetryableErrorsCounter.increment(); | ||
| } | ||
| } else { | ||
| retryableErrorsCounter.increment(); |
There was a problem hiding this comment.
Yea, this is a good point. I think we should assume non-retryable by default and only retry for known retryable.
Also related to this: we should combine this logic with the retry logic. If somebody changes that logic, these metrics may be missed.
4 participants
Description
Add Retryable/Non-Retryable Exception + API Calls Metrics for O365 Plugin
Issues Resolved
Not related to an existing issue, but will be helpful to have these new metrics.
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.
Signed-off-by: Brett Zeligson zeligsonbrett@gmail.com