Backporting Transition to JSON logging to V1 #1802
Conversation
davidmrdavid
requested review from
ConnorMcMahon,
cgillum,
amdeel and
bachuv
and removed request for
ConnorMcMahon and
cgillum
|
This still needs E2E testing, but I'll complete that prior to a merge |
|
I seem to recall E2E testing this, but it's been a while, so I can do it again. Still, I'd appreciate a conditional approval :) |
|
I'm not sure if this has been covered already in other PR comments, but I noticed that the v1 branch points to an old version of DurableTask.AzureStorage: <PackageReference Include="Microsoft.Azure.DurableTask.AzureStorage" Version="1.7.7" />Can we update this to point to the latest (currently 1.8.5)? |
|
@davidmrdavid, we ready to do final validation on this with 1.8.5 of DurableTask.AzureStorage now that #1838 was merged? |
|
@ConnorMcMahon, yes but I'll need to pull in some changes and re-push. I'm responding to feedback in a different PR, but I'll get back to this shortly, I'll send you a re-review request once this PR is ready to be validated once more. |
test/Common/TestHelpers.cs
Outdated
| "Pid", | ||
| "Tid", | ||
| }; | ||
| List<string> keys = json.Properties().Select(p => p.Name).ToList(); |
There was a problem hiding this comment.
nit: HashSet<string> would be more appropriate than List<string>. That or just do json.TryGetValue(expectedKey, out _) and avoid the extra data structure altogether.
test/Common/TestHelpers.cs
Outdated
| file.Add(streamReader.ReadLine()); | ||
| } | ||
|
|
||
| return file.ToArray(); |
There was a problem hiding this comment.
nit: This isn't important for test code, but it would be more efficient just to return file rather than call ToArray() on it (which copies all the elements).
| "ProviderName", | ||
| "TaskName", | ||
| "EventId", | ||
| "EventTimestamp", |
There was a problem hiding this comment.
Probably out of scope for this PR, but with this change (or with changes that preceded it) will we get fractional seconds just like we do with Windows? Not being able to properly sort by timestamps has been killing me when doing debugging on Linux. :)
There was a problem hiding this comment.
Ah yes, that's killing me as well. Unfortunately, the answer is no, and I explain more about why in here: #1843
Let's use that issue to track and discuss this work-item :)
|
Just e2e tested this again and it works. Merging! |
Tackles: #1733
Backports: #1721
Issue describing the changes in this PR
resolves #1733