poll function logs if the app is dynamic linux. Closes #2571 #2584

ahmelsayed · 2018-04-18T23:20:16Z

No description provided.

ahmelsayed · 2018-04-18T23:21:20Z

client/src/app/function-dev/function-dev.component.ts

+
+
+    updateLastInteraction() {
+        this.lastFunctionDevInteraction = new Date();


this is sort of a hack I think. The other way I think it can be done is to broadcast an event that streaming logs subscribes to. @ehamai what do you think is the right approach here?

actually never mind. I think it should be an event. I need to use it somewhere else too. I'll update the PR

+1 for event

@nertim I updated the PR

ammanifold · 2018-04-18T23:25:52Z

client/src/app/shared/services/function-app.service.ts

@@ -581,25 +581,33 @@ export class FunctionAppService {
                .map(r => r.json() as HostStatus));
    }

-    getOldLogs(context: FunctionAppContext, fi: FunctionInfo, range: number): Result<string> {
+    getLogs(context: FunctionAppContext, fi: FunctionInfo, range?: number, force = false): Result<string> {


range [](start = 59, length = 5)

Why is range optional now?

This method used to be only used before we start streaming request for logs.
The range was used to retrieve the last range bytes from the latest log file. That way when you switch to a new function, you get to see the last range bytes of the log file then the log streaming starts after it.

When polling, we don't need to pass a range, since we can just get the whole last file and update the UI with the content of that file. It makes the behavior slightly different between polling and streaming logs, but I think that's okay. The goal is to eventually remove both approaches and have an API that will return the set of logs for a given invocation, and use the Errors and Warnings for compilation errors.

would it be better to split the function in that case? getPollingLogs and getStreamingLogs ? factor out the common pieces in helper methods. Like that we can avoid branching within this method.

I'm not sure the labels (getPollingLogs, getStreamingLogs) would make sense here since it's more of the caller who is either polling or streaming. This method is more generic in that it just retrieves the latest log either with a range or not.

maybe rename getLogs to getLatestFunctionLogs?

ammanifold · 2018-04-18T23:26:59Z

client/src/app/function-dev/function-dev.component.ts

@@ -742,4 +748,9 @@ export class FunctionDevComponent extends FunctionAppContextComponent implements
                }
            });
    }
+
+


nit extra line

nertim · 2018-04-19T00:38:57Z

client/src/app/shared/services/function-app.service.ts

@@ -581,25 +581,33 @@ export class FunctionAppService {
                .map(r => r.json() as HostStatus));
    }

-    getOldLogs(context: FunctionAppContext, fi: FunctionInfo, range: number): Result<string> {
+    getLogs(context: FunctionAppContext, fi: FunctionInfo, range?: number, force = false): Result<string> {


nit. force: boolean = false

sure, though TypeScript can infer that

nertim · 2018-04-19T00:42:49Z

client/src/app/log-streaming/log-streaming.component.ts

+     * @param newLogs string to use to update the update the displayed logs with
+     * @param oldLogs logs to prepend to newLogs. This is only used in streaming scenario
+     */
+    private updateLogs(newLogs: string, oldLogs?: string): number {


nit. prefix with '_'

nertim · 2018-04-19T00:52:09Z

client/src/app/log-streaming/log-streaming.component.ts

        });

-        return promise;
+        return newLogs.length + (oldLogs || '').length;


newLogs.length + (oldLogs || '') [](start = 15, length = 32)

should this be this.log.length?

yeah, I suspect that you're probably right. Though this is the existing logic. I just tried to reduce copy-paste so I moved it to it's own function. I'd rather not change it because there is a lot of odd math going on to handle some random cases we had before and it's not well documented why it's this way (I added all that logic a time ago and don't remember all the intricacies about why it's the way it's now)

Even at the time I added all that, the plan was for it to be temporary until we get an API that gives us the logs for an invocation. We are probably closer now, so I'm tempted to leave this funny math as is until that happend.

nertim · 2018-04-19T00:56:07Z

client/src/app/log-streaming/log-streaming.component.ts

+        } else {
+            this.log = oldLogs
+                ? oldLogs + newLogs.substring(newLogs.indexOf('\n') + 1)
+                : newLogs;


should these logs get truncated based on the maxCharsInLog?

Yes.

The only "overage" that can happen is if oldLogs + newLogs are > maxCharactersInLog but we know that oldLogs are at the most 10,000 bytes (or 10,000 characters, probably unless there is unicode in there) so I figured 500,000 +- 10,000 isn't that much off.

All the math in here is too complicated and we should simplify it once there is a specific logs API. I'll update this to truncate based on maxChars though

nertim

Sorry for all the questions :(...

I think there is a lot of math happening in a lot of places.

nertim · 2018-04-19T01:00:26Z

client/src/app/log-streaming/log-streaming.component.ts

+                const nextInterval = diff - oldLogs.length > intervalIncreaseThreshold ? this.timerInterval + defaultInterval : this.timerInterval - defaultInterval;
+                if (nextInterval < defaultInterval) {
+                    this.timerInterval = defaultInterval;
+                } else if (nextInterval > maxInterval) {
                    this.timerInterval = defaultInterval;


defaultInterval [](start = 41, length = 15)

This is existing code, but is this correct? if so, can we combine the two if statements?

if (next < default || next > max)

you're right

ahmelsayed · 2018-04-19T02:21:33Z

@nertim so since I'm mainly trying to reduce any regressions to what's there today (especially since the math is pretty complicated), I changed the PR to not touch existing logic in the streaming case, and just add new logic for the polling case. #Resolved

nertim · 2018-04-19T03:28:15Z

sounds good.

In reply to: 382588602 [](ancestors = 382588602)

nertim

Thank you for your patience with my comments Ahmed! :)

ehamai · 2018-04-19T14:38:05Z

client/src/app/log-streaming/log-streaming.component.ts

+            // if logging isn't stopped
+            .filter(() => !this.stopped)
+            // and until this.pollingActive OR 5 minutes have passed, which ever comes first
+            .takeUntil(this.pollingActive$.merge(Observable.timer(5 * 60 * 1000)))


.takeUntil(this.pollingActive$.merge(Observable.timer(5 * 60 * 1000))) [](start = 12, length = 70)

nice!

ehamai · 2018-04-19T14:50:23Z

client/src/app/log-streaming/log-streaming.component.ts

@@ -174,7 +230,7 @@ export class LogStreamingComponent extends FunctionAppContextComponent implement
            this.xhReq.setRequestHeader('FunctionsPortal', '1');
            this.xhReq.send(null);
            if (!createEmpty) {
-                this._functionAppService.getOldLogs(this.context, this.functionInfo, 10000).subscribe(r => oldLogs = r.result);
+                this._functionAppService.getLogs(this.context, this.functionInfo, 10000).subscribe(r => oldLogs = r.result);
            }


It's probably helpful to explain why you might be doing this even in the streaming case.

I added a comment explaining

ehamai

ahmelsayed requested review from nertim, ehamai, ammanifold and hartra344 April 18, 2018 23:20

ahmelsayed commented Apr 18, 2018

View reviewed changes

ammanifold reviewed Apr 18, 2018

View reviewed changes

ahmelsayed force-pushed the ahmels-2571 branch from 38deff3 to 36c79f4 Compare April 18, 2018 23:56

nertim reviewed Apr 19, 2018

View reviewed changes

nertim approved these changes Apr 19, 2018

View reviewed changes

nertim reviewed Apr 19, 2018

View reviewed changes

nertim suggested changes Apr 19, 2018

View reviewed changes

nertim reviewed Apr 19, 2018

View reviewed changes

ahmelsayed force-pushed the ahmels-2571 branch from 3fc3390 to e9c551e Compare April 19, 2018 02:27

nertim approved these changes Apr 19, 2018

View reviewed changes

ehamai reviewed Apr 19, 2018

View reviewed changes

ehamai approved these changes Apr 19, 2018

View reviewed changes

poll function logs if the app is dynamic linux. Closes #2571

bf2264e

ahmelsayed force-pushed the ahmels-2571 branch from 71aac87 to bf2264e Compare April 19, 2018 16:59

ahmelsayed merged commit 7b97e0e into dev Apr 19, 2018

ahmelsayed deleted the ahmels-2571 branch April 19, 2018 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

poll function logs if the app is dynamic linux. Closes #2571 #2584

poll function logs if the app is dynamic linux. Closes #2571 #2584

ahmelsayed commented Apr 18, 2018

ahmelsayed Apr 18, 2018

ahmelsayed Apr 18, 2018

nertim Apr 18, 2018

ahmelsayed Apr 19, 2018

ammanifold Apr 18, 2018

ahmelsayed Apr 19, 2018

nertim Apr 19, 2018

ahmelsayed Apr 19, 2018

ahmelsayed Apr 19, 2018 •

edited

ammanifold Apr 18, 2018

nertim Apr 19, 2018

ahmelsayed Apr 19, 2018

nertim Apr 19, 2018

nertim Apr 19, 2018

ahmelsayed Apr 19, 2018

nertim Apr 19, 2018

ahmelsayed Apr 19, 2018

nertim left a comment

nertim Apr 19, 2018

ahmelsayed Apr 19, 2018

ahmelsayed commented Apr 19, 2018 •

edited by nertim

nertim commented Apr 19, 2018

nertim left a comment

ehamai Apr 19, 2018

ehamai Apr 19, 2018

ahmelsayed Apr 19, 2018

ehamai left a comment



		updateLastInteraction() {
		this.lastFunctionDevInteraction = new Date();

poll function logs if the app is dynamic linux. Closes #2571 #2584

poll function logs if the app is dynamic linux. Closes #2571 #2584

Conversation

ahmelsayed commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmelsayed Apr 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nertim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmelsayed commented Apr 19, 2018 • edited by nertim

nertim commented Apr 19, 2018

nertim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehamai left a comment

Choose a reason for hiding this comment

ahmelsayed Apr 19, 2018 •

edited

ahmelsayed commented Apr 19, 2018 •

edited by nertim