Replace the impl of Syscall.rename and remove Mono.Posix.NETStandard #1641
Conversation
|
As for adding unit tests for this, we don't have a linux enviroment in our durable-extension pipeline, do we? If we did, I could add an inotify test |
|
We could add a step to the smoke test validation step for release to make sure that high volume traffic on Linux doesn't miss data? Not sure how easy that would be and how much value it would have, but without substantial work, we definitely can't unit test this right now (we use windows workers on our CI). |
| #endif | ||
|
|
||
| } | ||
|
|
||
| #if !FUNCTIONS_V1 | ||
| [DllImport("libc", SetLastError = true)] |
There was a problem hiding this comment.
How does [DllImport] work if the library is not available (for instance, on Windows)?
Does this dynamically try to import this functionality when rename is actually called? In that case, this likely isn't an issue since this logger should never actually be used on Windows.
There was a problem hiding this comment.
My understanding is that DllImport will not actually seek to invoke any system-level libraries until rename is called, so this shouldn't be an issue.
From this SO answer: https://stackoverflow.com/questions/39144458/what-happens-if-i-have-dllimport-c-net-for-an-api-not-supported-by-the-opera
There was a problem hiding this comment.
In that case, we should decide on whether in the degenerate case this logger class is used on a non-Linux platform if we fail fast with an exception, or we should wrap the call to rename() in a try-catch.
@cgillum, what are your thoughts in this scenario? I normally lean towards just letting the exception propagate, but since this is logging code I'm never quite sure of proper error handling.
There was a problem hiding this comment.
I +1 failing fast if we're somehow using the logger in windows
There was a problem hiding this comment.
Don't we have code that always ensures we're only running this logger in Linux? I'm fine with letting it fail on Windows, though I recommend we log the failure before rethrowing any exception so that someone debugging will have a bit more context.
There was a problem hiding this comment.
So the suggestion is to: catch, then log, then re-throw? Got it :)
|
Merging this into |
* Replace the impl of Syscall.rename and remove Mono.Posix.NETStandard (follow-up to #1641) (#1643) Co-authored-by: Tatsuro Shibamura <me@shibayan.jp> * Adding Missing Fields to Linux Logger: ProviderName, TaskName, ActivityId, RelatedActivityId, StampName, etc (#1515) * Linux Logger now supports the legacy regex logging path (#1522) "It's done, it's finally done" - Frodo Co-authored-by: David Justo <dajusto@microsoft.com> Co-authored-by: Chris Gillum <cgillum@microsoft.com> * Replace the impl of Syscall.rename and remove Mono.Posix.NETStandard (follow-up to #1641) (#1643) Co-authored-by: Tatsuro Shibamura <me@shibayan.jp> * Shift tracing inside conditional check * Added Tracing options, project compiles * Disable verbosity check in V1 * Remove unecessary xml file, remove release notes, update release version, update azurestorage * Update azurestorage refs in test csprojs * downgrade back to azurestorage 1.7.7 Co-authored-by: Tatsuro Shibamura <me@shibayan.jp> Co-authored-by: David Justo <dajusto@microsoft.com> Co-authored-by: Chris Gillum <cgillum@microsoft.com>
Mono.Posix.NETStandard was included since Durable Functions v2.3.1, which increased the size of the deployment package by nearly three times.
This is a contribution by @shibayan :)
And I have verified that this implementation still raises the correct
inotifysignal, so our telemetry pipeline should continue to work with itIssue describing the changes in this PR
#1597
resolves #1597
Pull request checklist
pending_docs.mdrelease_notes.md