Skip to content
This repository has been archived by the owner on Mar 1, 2022. It is now read-only.

Added better error handling in LogglySink #55

Merged
merged 3 commits into from
Aug 18, 2020
Merged

Added better error handling in LogglySink #55

merged 3 commits into from
Aug 18, 2020

Conversation

danjagnow
Copy link
Contributor

Updated the EmitBatchAsync method in LogglySink to handle potential exceptions and evaluate the LogResponse when calling the Log method of the LogglyClient. It now writes to SelfLog when problems occur to enable easier debugging.

I believe this may help with the problems described in #39.

Also removed a digit from the PackageReference for Serilog.Sinks.RollingFileAlternate in sampleDurableLogger.csproj to get local builds working.

Removed a digit from the PackageReference for Serilog.Sinks.RollingFileAlternate in sampleDurableLogger.csproj to get local builds working.
Updated the EmitBatchAsync method in LogglySink to handle potential exceptions and evaluate the LogResponse when calling the Log method of the LogglyClient.  It now writes to SelfLog when problems occur to enable easier debugging.
@danjagnow
Copy link
Contributor Author

@nblumhardt or @MiguelAlho, just wondering if you've had a chance to look at this PR. I know life gets busy. 😄

Copy link
Contributor

@nblumhardt nblumhardt left a comment

Choose a reason for hiding this comment

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

Thanks for the nudge! The response code logging looks like a great improvement. Exception handling in sinks is discouraged because exceptions are (should be!) handled centrally and consistently by Serilog.

break;
}
}
catch (Exception ex)
Copy link
Contributor

Choose a reason for hiding this comment

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

The exception shouldn't be caught here, but rather propagate up to Serilog (just removing the try/catch block here is fine).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks for taking a look!

Removed the try/catch block in the EmitBatchAsync method of LogglySink to allow Serilog to handle exceptions centrally, based on PR feedback.
@danjagnow danjagnow requested a review from nblumhardt August 18, 2020 23:55
@nblumhardt nblumhardt merged commit f30e389 into serilog-archive:dev Aug 18, 2020
@nblumhardt
Copy link
Contributor

Thanks! 👍

@danjagnow danjagnow deleted the bugfix/errors-to-selflog branch August 19, 2020 00:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants