Initial implementation for workflow log tracing#1176
Conversation
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1176 +/- ##
=======================================
Coverage 66.42% 66.42%
=======================================
Files 171 171
Lines 5758 5758
Branches 626 626
=======================================
Hits 3825 3825
Misses 1784 1784
Partials 149 149
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
| } | ||
| } | ||
| } | ||
| }, cts.Token); |
There was a problem hiding this comment.
AFAIK, this CancellationToken is only used if the Task has not yet already started, which it most likely has, so it's possible that the task never ends (in the case of not all expected logs being found).
You could use the async method Stream.ReadLineAsync(CancellationToken) to provide a means of abandoning the search when time expires. That would also allow you to avoid calling Task.WaitAll(), which is a synchronously-blocking method.
| } | ||
| }, cts.Token); | ||
|
|
||
| if (!Task.WaitAll(new Task[] {searchTask}, timeout)) |
There was a problem hiding this comment.
Nit: maybe hold off testing the result until after the log file has been deleted (rather than two deletion paths).
halspang
left a comment
halspang
left a comment
There was a problem hiding this comment.
Can you give some examples of what logs are output when we run a workflow?
| this.logger.Log(LogLevel.Information, "List of registered workflows"); | ||
| foreach (string item in registeredWorkflows) | ||
| { | ||
| this.logger.Log(LogLevel.Information, item); | ||
| } | ||
|
|
||
| this.logger.Log(LogLevel.Information, "List of registered activities:"); | ||
| foreach (string item in registeredActivities) | ||
| { | ||
| this.logger.Log(LogLevel.Information, item); | ||
| } |
There was a problem hiding this comment.
Nit: Indentation is weird here.
| /// <summary> | ||
| /// Defines runtime options for workflows. | ||
| /// </summary> | ||
| internal sealed class WorkflowLoggingService : IHostedService |
There was a problem hiding this comment.
I'm not sure if hosted service is the right way to go here. I was thinking more that this would be injected into the various workflow runtime mechanics, like the context.
| using (StreamReader reader = new StreamReader(logFilePath)) | ||
| { | ||
| string line; | ||
| while ((line = await reader.ReadLineAsync()) != null) |
There was a problem hiding this comment.
Need to pass the cancellation token to ReadLineAsync() so it will stop reading after the timeout.
| finally | ||
| { | ||
| File.Delete(logFilePath); | ||
| if (!allLogsFound) |
There was a problem hiding this comment.
The allLogsFound test should probably be outside/after the finally block; it doesn't need to be executed in the case of an exception (e.g. timeout) as the exception will bubble up and fail the test.
RyanLettieri
force-pushed
the
workflow-log-tracing
branch
from
0bff075 to
71064cf
Compare
* Initial setup for workflow log tracing Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com> * Created log sink for E2E tests using serilog Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com> * Formatting Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com> * Addressing feedback on review Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com> * Addressing some review comments Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com> * Addressing more feedbck in the workflow e2e test Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com> --------- Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com> Signed-off-by: Divya Perumal <diperuma@microsoft.com>
3 participants
Description
Added in basic logging for workflow to log the registered activities and registered workflows.
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #[1170]
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: