Support ILoggingFailureListener
#342
Conversation
…chnically a breaking change
| SelfLog.WriteLine("Log event {0} was lost since it was not possible to open the file or create a new one.", logEvent.RenderMessage()); | ||
| } | ||
| */ | ||
| failed = _currentFile == null; |
There was a problem hiding this comment.
@Falco20019 this should cover the TODO
There was a problem hiding this comment.
I will have a look beginning next week, thanks 🎉
There was a problem hiding this comment.
@nblumhardt There is sadly no artifact to use on AppVeyor as the build "failed successfully" ;)
https://ci.appveyor.com/project/serilog/serilog-sinks-file/builds/51655364
I will create a local build for testing for now, but you might want to fix the CI pipeline if you intend to use Actions + AppVeyor in parallel. If you are dropping AppVeyor with this PR as-well, I would add
There was a problem hiding this comment.
Did some testing and seems to work as expected. Thanks!
Could you still have a hotfix-release for #337 before having this major release? Our company policy requires some effort on switching major version, so for getting the bug fixed first on our current version would help me a lot!
| $branch = @{ $true = $env:CI_TARGET_BRANCH; $false = $(git symbolic-ref --short -q HEAD) }[$NULL -ne $env:CI_TARGET_BRANCH]; | ||
| $revision = @{ $true = "{0:00000}" -f [convert]::ToInt32("0" + $env:CI_BUILD_NUMBER, 10); $false = "local" }[$NULL -ne $env:CI_BUILD_NUMBER]; | ||
| $suffix = @{ $true = ""; $false = "$($branch.Substring(0, [math]::Min(10,$branch.Length)) -replace '([^a-zA-Z0-9\-]*)', '')-$revision"}[$branch -eq "main" -and $revision -ne "local"] | ||
| $commitHash = $(git rev-parse --short HEAD) | ||
| $buildSuffix = @{ $true = "$($suffix)-$($commitHash)"; $false = "$($branch)-$($commitHash)" }[$suffix -ne ""] |
There was a problem hiding this comment.
NIT: Only a suggestion to comply better with SemVer 2.0.0
https://learn.microsoft.com/en-us/nuget/concepts/package-versioning?tabs=semver20sort#pre-release-versions suggests to use the version in the format 7.0.0-selflog-on.1+04ac85d instead of 7.0.0-selflog-on-00001-04ac85d. Doing this will also allow you to just use $suffix instead of differentiating with $buildSuffix since the nuget package (and any relevant other part) will drop the metadata part as documented by https://learn.microsoft.com/en-us/nuget/concepts/package-versioning?tabs=semver20sort#normalized-version-numbers
Using the . instead of - before the CI number also signifies it's a numeric and should be sorted as such. NuGet/VisualStudio/... is aware of that, which makes it nicer than padding it to 5 digits. This is widely supported since VS 2017.
| $branch = @{ $true = $env:CI_TARGET_BRANCH; $false = $(git symbolic-ref --short -q HEAD) }[$NULL -ne $env:CI_TARGET_BRANCH]; | |
| $revision = @{ $true = "{0:00000}" -f [convert]::ToInt32("0" + $env:CI_BUILD_NUMBER, 10); $false = "local" }[$NULL -ne $env:CI_BUILD_NUMBER]; | |
| $suffix = @{ $true = ""; $false = "$($branch.Substring(0, [math]::Min(10,$branch.Length)) -replace '([^a-zA-Z0-9\-]*)', '')-$revision"}[$branch -eq "main" -and $revision -ne "local"] | |
| $commitHash = $(git rev-parse --short HEAD) | |
| $buildSuffix = @{ $true = "$($suffix)-$($commitHash)"; $false = "$($branch)-$($commitHash)" }[$suffix -ne ""] | |
| $branch = @{ $true = $env:CI_TARGET_BRANCH; $false = $(git symbolic-ref --short -q HEAD) }[$NULL -ne $env:CI_TARGET_BRANCH]; | |
| $revision = @{ $true = "{0}" -f [convert]::ToInt32("0" + $env:CI_BUILD_NUMBER, 10); $false = "local" }[$NULL -ne $env:CI_BUILD_NUMBER]; | |
| $suffix = @{ $true = ""; $false = "$($branch.Substring(0, [math]::Min(10,$branch.Length)) -replace '([^a-zA-Z0-9\-]*)', '').$revision"}[$branch -eq "main" -and $revision -ne "local"] | |
| $buildSuffix = @{ $true = "$($suffix)"; $false = "$($branch)" }[$suffix -ne ""] |
There was a problem hiding this comment.
Personally I would use 7.0.0-ci.1+04ac85d-selflog-on-open-failure, but tried to keep the suggestion closer to what you do right now.
There was a problem hiding this comment.
Additionally, there seems to be something in place that adds the full revision anyway:
The result locally was: 7.0.0-selflog-on.1+04ac85d60ddb0e109312ca820a113b599d286dc9 with my change and 7.0.0-selflog-on-00001-04ac85d+04ac85d60ddb0e109312ca820a113b599d286dc9 with your current code.
I did check if this was local-only or also from the official packages, and the latest version on NuGet is 6.0.0+c7b9bc3d87f9bb98572a52286135efda1e608ad6.
There was a problem hiding this comment.
Thanks for digging into this! These scripts are from the upstream serilog/serilog and we're trying to avoid configuration drift by making any changes up there. I agree this could be better 👍 - I'll leave this as-is here but take a look in the next round of maintenance on the Serilog package.
| return false; | ||
| } | ||
| } | ||
| catch (AbandonedMutexException) | ||
| { | ||
| SelfLog.WriteLine("Inherited shared file mutex after abandonment by another process"); | ||
| SelfLog.WriteLine("inherited the shared file mutex after abandonment by another process"); |
There was a problem hiding this comment.
Is this intentionally sticking with SelfLog directly? Just asking as it's the only call to SelfLog.WriteLine that remained.
There was a problem hiding this comment.
Thanks for taking a look. Yes, this one doesn't go through the failure listener because it's not a failure. In this situation, another process failed, but we've picked up the mutex and we're carrying on.
There was a problem hiding this comment.
I do like the concept of having every log still appearing somewhere if everything went wrong. Thanks!
|
@Falco20019 thanks for taking a look a this. I am not sure about the timeline for releasing a patch, it's not hugely likely we'd push a point release out, as some additional work would be needed to get the original AppVeyor build through, and each new version causes some additional effort for downstream consumers. Since |
|
It's fine, I just asked in case you are fine with it. As we are using an internal NuGet Repository, I will just publish a hotfix version there to be used and switch back to official releases in November. Not the first time we have to do it and surely not the last. |
Seals
PeriodicFlushToDiskSink, so technically a breaking change.Includes a switch to Actions for CI.