Initial implementation for workflow log tracing #1176
Conversation
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
…flow-log-tracing
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>
…flow-log-tracing
| } | ||
| } | ||
| } | ||
| }, 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).
| 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
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: