[CosmosDb] Avoid swallowing exceptions when retrieving votes#973
Conversation
| } | ||
| catch (Exception e) when (e is CosmosException || e is InvalidOperationException) | ||
| { | ||
| if (!TryHandleCosmosException(e)) |
There was a problem hiding this comment.
From the links you shared Alexey, we have upstream handling already for any exceptions coming from this code on the scale monitor path. My primary concern with this was whether letting these through was going to start failing other scenarios that are currently passing (i.e. regression). I don't know who else may be using these APIs.
| errormsg = e.ToString(); | ||
| } | ||
|
|
||
| _logger.LogWarning(Events.OnScaling, errormsg); |
There was a problem hiding this comment.
I don't understand - why won't these error messages written to the supplied ILogger go to the customer's App Insights or other log streams? The logger should already be configured to do this.
jviau
left a comment
There was a problem hiding this comment.
Changing this exception logic is a high risk of a breaking change. While this class is internal, the exception could now bubble up into public API and customer code depending on how it is used.
Unless we can verify 100% this will never impact a customer scenario I don't think we can take it. And I don't mean just specific functions scenario, I mean any customer using any public API in this package, functions or not.
|
Closing in favor - #975 |
Proposed Improvement
We want to emit error logs to the customer’s Application Insights whenever there are connectivity issues to the resource.
Current Behavior
Exceptions are swallowed in the extension code and logged only in internal logs.
Planned Change
References
-https://github.com/Azure/azure-webjobs-sdk/blob/dev/src/Microsoft.Azure.WebJobs.Host/Scale/ScaleManager.cs#L195
-https://github.com/Azure/azure-webjobs-sdk/blob/dev/src/Microsoft.Azure.WebJobs.Host/Scale/ScaleManager.cs#L121
Additional Notes