Update `GetModuleLogs` method when `tail + since + until` options are provided. #4922

yophilav · 2021-04-30T04:45:40Z

TL;DR - This PR chances the behavior of GetModuleLogs method to better facilitate the portal experience.

Previous, when the GetModuleLogs is triggered from the azure portal with tail + since + until, all the 3 options are passed to Docker API management to fetch the log. The Docker API does the following:

Fetch the log of the module
Apply tail to the log
Apply the since and until to the log
This results in an empty log most of the time which is not a desirable behavior.

This PR changes the GetModuleLogs when the tail + since + until are specified together by doing the following:

Fetch Docker API with since + until log option
tail the returning log using LogProcessor (post-Akka).
This allows a customer to get a tail of log within a given time frame.

Note: This PR doesn't change the behavior of any other option combinations (i.e. tail + since, tail, since, etc). These combination are still managed by Docker API.

Note2: I set RequestHandle timeout to be 30s instead of 600s by default

…nted

yophilav · 2021-05-07T18:25:10Z

edge-agent/src/Microsoft.Azure.Devices.Edge.Agent.Service/Program.cs

@@ -255,7 +255,7 @@ public static async Task<int> MainAsync(IConfiguration configuration)
                {
                    case "twin":
                        bool enableStreams = configuration.GetValue(Constants.EnableStreams, false);
-                        int requestTimeoutSecs = configuration.GetValue(Constants.RequestTimeoutSecs, 600);
+                        int requestTimeoutSecs = configuration.GetValue(Constants.RequestTimeoutSecs, 30);


This is the part that I'm not super sure about if I should change the default request timeout from 600s to 30s for all the default. I don't really have a clean way to set a particular GetModuleLogs DM's default request timeout to be 30s. Any suggestions?

dm call has a timeout that can be set to ResponseTimeout in MethodRequest class. The default change is a big difference, it might cause issues with slower connections I assume.

ancaantochi · 2021-05-10T18:51:38Z

edge-agent/src/Microsoft.Azure.Devices.Edge.Agent.Core/logs/LogsProcessor.cs

+            return filter.Tail.Match<IReadOnlyList<ModuleLogMessage>>(
+                t =>
+                {
+                    return result.Skip(Math.Max(0, result.Count - t)).ToList().AsReadOnly();


nit: this looks like Math.Abs can be used instead

Unfortunately, I don't think Math.Abs() will work here. More particularly in case t > result.Count, Abs() would return the difference.. that's not what I want.

ancaantochi · 2021-05-10T18:56:09Z

...Microsoft.Azure.Devices.Edge.Agent.Edgelet/versioning/ModuleManagementHttpClientVersioned.cs

                since.ForEach(s => logsUrl.AppendFormat($"&{LogsUrlSinceParameter}={Uri.EscapeUriString(s)}"));
                until.ForEach(u => logsUrl.AppendFormat($"&{LogsUrlUntilParameter}={Uri.EscapeUriString(u)}"));
+
+                if (!(tail.HasValue && since.HasValue && until.HasValue))


does tail.HasValue need to be checked here? it looks like only since and until have to be checked that are not set

Yup, absolutely. This is where we about to send the docker API request via mgmt.sock.
Starting from this PR, the behavior is separated into 2 scenarios:

When all 3 options (tail + since + until) are specified, only the since + until is passed down to mgmt.sock

When other combination of options (tail+since, tail+until, until+since) are specified, the options are then passthrough edgeAgent into mgmt.sock directly.

Once the log from (1) is returned, the result is tail-ed after Akka parsing to enforce the tail.

ancaantochi · 2021-05-10T19:02:05Z

edge-agent/src/Microsoft.Azure.Devices.Edge.Agent.Service/Program.cs

@@ -255,7 +255,7 @@ public static async Task<int> MainAsync(IConfiguration configuration)
                {
                    case "twin":
                        bool enableStreams = configuration.GetValue(Constants.EnableStreams, false);
-                        int requestTimeoutSecs = configuration.GetValue(Constants.RequestTimeoutSecs, 600);
+                        int requestTimeoutSecs = configuration.GetValue(Constants.RequestTimeoutSecs, 30);


dm call has a timeout that can be set to ResponseTimeout in MethodRequest class. The default change is a big difference, it might cause issues with slower connections I assume.

… provided. (Azure#4922) TL;DR - This PR chances the behavior of `GetModuleLogs` method to better facilitate the portal experience. Previous, when the `GetModuleLogs` is triggered from the azure portal with `tail + since + until`, all the 3 options are passed to Docker API management to fetch the log. The Docker API does the following: 1. Fetch the log of the module 2. Apply `tail` to the log 3. Apply the `since` and `until` to the log This results in an empty log most of the time which is not a desirable behavior. This PR changes the `GetModuleLogs` when the `tail + since + until` are specified together by doing the following: 1. Fetch Docker API with `since + until` log option 2. `tail` the returning log using LogProcessor (post-Akka). This allows a customer to get a tail of log within a given time frame. Note: This PR doesn't change the behavior of any other option combinations (i.e. `tail + since`, `tail`, `since`, etc). These combination are still managed by Docker API. Note2: I set RequestHandle timeout to be 30s instead of 600s by default

… provided (#4922) (#5171) TL;DR - This PR chances the behavior of `GetModuleLogs` method to better facilitate the portal experience. Previous, when the `GetModuleLogs` is triggered from the azure portal with `tail + since + until`, all the 3 options are passed to Docker API management to fetch the log. The Docker API does the following: 1. Fetch the log of the module 2. Apply `tail` to the log 3. Apply the `since` and `until` to the log This results in an empty log most of the time which is not a desirable behavior. This PR changes the `GetModuleLogs` when the `tail + since + until` are specified together by doing the following: 1. Fetch Docker API with `since + until` log option 2. `tail` the returning log using LogProcessor (post-Akka). This allows a customer to get a tail of log within a given time frame. Note: This PR doesn't change the behavior of any other option combinations (i.e. `tail + since`, `tail`, `since`, etc). These combination are still managed by Docker API. Note2: I set RequestHandle timeout to be 30s instead of 600s by default

yophilav added 10 commits April 26, 2021 15:01

Testing

bbf003a

Seek

f3c7075

Messy but working

ef7182e

Unsupported seek

fa0d6d7

Re organized

adb925b

Update per comments

b59d278

Update the default timeout to 30s & only tail with both options prese…

3d1419a

…nted

Looking

36f810d

Sometimes the code just stare right at you but you just don't see it

10ae52e

Clean up

ef549ac

yophilav requested review from varunpuranik and ancaantochi May 3, 2021 21:20

Merge branch 'release/1.2' into yophilav/GetModuleLogsIssue1_1.2

c9837b7

yophilav marked this pull request as ready for review May 3, 2021 21:20

yophilav changed the title ~~[DRAFT] Update docker's log tail feature when since and until is applied.~~ Update GetModuleLogs method when tail + since + until options are provided. May 3, 2021

Merge branch 'release/1.2' into yophilav/GetModuleLogsIssue1_1.2

e2279d1

yophilav commented May 7, 2021

View reviewed changes

ancaantochi reviewed May 10, 2021

View reviewed changes

yophilav and others added 2 commits May 10, 2021 16:11

Undo the default timeout

233593c

Merge branch 'release/1.2' into yophilav/GetModuleLogsIssue1_1.2

e714b32

yophilav added the ready-to-merge label May 10, 2021

ancaantochi approved these changes May 11, 2021

View reviewed changes

kodiakhq bot merged commit 2b650a8 into Azure:release/1.2 May 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `GetModuleLogs` method when `tail + since + until` options are provided. #4922

Update `GetModuleLogs` method when `tail + since + until` options are provided. #4922

yophilav commented Apr 30, 2021 •

edited

yophilav May 7, 2021

ancaantochi May 10, 2021

ancaantochi May 10, 2021

yophilav May 10, 2021

ancaantochi May 10, 2021

yophilav May 10, 2021

ancaantochi May 10, 2021

Update GetModuleLogs method when tail + since + until options are provided. #4922

Update GetModuleLogs method when tail + since + until options are provided. #4922

Conversation

yophilav commented Apr 30, 2021 • edited

yophilav May 7, 2021

Choose a reason for hiding this comment

ancaantochi May 10, 2021

Choose a reason for hiding this comment

ancaantochi May 10, 2021

Choose a reason for hiding this comment

yophilav May 10, 2021

Choose a reason for hiding this comment

ancaantochi May 10, 2021

Choose a reason for hiding this comment

yophilav May 10, 2021

Choose a reason for hiding this comment

ancaantochi May 10, 2021

Choose a reason for hiding this comment

Update `GetModuleLogs` method when `tail + since + until` options are provided. #4922

Update `GetModuleLogs` method when `tail + since + until` options are provided. #4922

yophilav commented Apr 30, 2021 •

edited