Re: [pve-devel] [PATCH widget-toolkit] fix #3971: Download and copy button in the TaskViewer

[Daniel Tschlatscher <d.tschlatscher@proxmox.com>]

On 4/1/22 17:18, Thomas Lamprecht wrote:
> On 01.04.22 16:07, Daniel Tschlatscher wrote:
>> The taskviewer now has 2 more buttons which implement
>> functionality for downloading the current tasklog as a file
>> or copying it to the clipboard. The code for saving the log
>> to a file was taken from the pve System Report class and
>> moved to its own function in the Utils file.
> just fyi, 70cc is a good text-width limit for commit messages, neither too
> long nor too short is ideal to read, as said, fyi as I'm surely not going to
> nack stuff just based on to narrow limits though, albeit I may fix those up
> with an amend on apply ;-)
>
>> Tested on Firefox 91.7.0esr and Chromium Version 99
> Which products did you test? The task log viewer is available on PVE, PMG and
> PBS after all.
>
> Assuming it works on all products the semantics look OK, I see quite some
> improvements style wise and some chance to drop legacy code now, commented both
> inline.
>
>> Signed-off-by: Daniel Tschlatscher<d.tschlatscher@proxmox.com>
>> ---
>>   src/Utils.js             | 17 +++++++++++
>>   src/window/TaskViewer.js | 65 +++++++++++++++++++++++++++++++++++++++-
>>   2 files changed, 81 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/Utils.js b/src/Utils.js
>> index 6a03057..3fff3da 100644
>> --- a/src/Utils.js
>> +++ b/src/Utils.js
>> @@ -1272,6 +1272,23 @@ utilities: {
>>   	    .map(val => val.charCodeAt(0)),
>>   	);
>>       },
>> +
>> +    // Convert the given string to a file and "download" it locally
>> +    textToFile: function(fileName, fileContent) {
> I'd call that 'downloadTextAsFile' or maybe more JS "type" related:
> 'downloadStringAsFile' and s/fileContent/text/
>
>> +	// Internet Explorer
> I think as we dropped IE support with adopting modern es6 a while ago we can now
> drop such things too.
>
>> +	if (window.navigator.msSaveOrOpenBlob) {
>> +	    navigator.msSaveOrOpenBlob(new Blob([fileContent]), fileName);
>> +	} else {
>> +	    var element = document.createElement('a');
> please not only use `let` for local variables in new code but also transform
> existing code from var to let if you have to touch it anyway. `var` just has
> horrible scoping rules (becomes visible in the parent's block scope) and should
> be avoided on principle, if possible.
>
>> +	    element.setAttribute('href', 'data:text/plain;charset=utf-8,' +
>> +		encodeURIComponent(fileContent));
>> +	    element.setAttribute('download', fileName);
>> +	    element.style.display = 'none';
>> +	    document.body.appendChild(element);
>> +	    element.click();
>> +	    document.body.removeChild(element);
> you do not need to append the created element to the body anymore, at least
> I didn't when adapting such a function for the PVE PBS-storage UI key-secret stuff.
>
> https://git.proxmox.com/?p=pve-manager.git;a=blob;f=www/manager6/storage/PBSEdit.js;h=b46ddf71cc5d#l87
>
>> +	}
>> +    },
>>   },
>>   
>>       singleton: true,
>> diff --git a/src/window/TaskViewer.js b/src/window/TaskViewer.js
>> index 996a41b..bf4533d 100644
>> --- a/src/window/TaskViewer.js
>> +++ b/src/window/TaskViewer.js
>> @@ -229,13 +229,75 @@ Ext.define('Proxmox.window.TaskViewer', {
>>   	    border: false,
>>   	});
>>   
>> +	let downloadBtn = new Ext.Button({
>> +		text: gettext('Download'),
> missing download icon class, also move handler here (see below)
>
>> +	});
>> +
>> +	let copyBtn = new Ext.Button({
>> +		text: gettext('Copy'),
>> +		iconCls: 'fa fa-clipboard',
>> +	});
>> +
>>   	let logView = Ext.create('Proxmox.panel.LogView', {
>>   	    title: gettext('Output'),
>> -	    tbar: [stop_btn2],
>> +	    tbar: [stop_btn2, '->', downloadBtn, copyBtn],
>>   	    border: false,
>>   	    url: "/api2/extjs/nodes/" + task.node + "/tasks/" + encodeURIComponent(me.upid) + "/log",
>>   	});
>>   
>> +	const downloadLogFull = function(callback) {> +	    Proxmox.Utils.API2Request({
>> +		url: "/nodes/" + task.node + "/tasks/"
>> +		    + encodeURIComponent(me.upid) + '/log',
> please use template strings for such things, shorter and easier to read, fits into a
> single 100cc line too.
>
>> +		waitMsgTarget: me,
>> +		method: 'GET',
>> +		params: {
>> +		    'limit': logView.viewModel.data.data.total,
>> +		},
>> +		failure: function(response, opts) {
>> +		    callback(response.htmlStatus, null);
>> +		},
> we normally use modern arrow fn for such edge-case oneliner's nowadays:
>
> failure: response => callback(response.htmlStatus, null),
>
> but actually, why even bother with a callback if all instances using this just Ext.alert
> the error anyway? Only complicates things here and in the instances too, just do:
>
> failure: response => Ext.Msg.alert(gettext('Error'), response.htmlStatus),
>
> and you can drop the error handling in the handlers too..
>
>> +		success: function(response) {
>> +		    let fileContent = "";
>> +
>> +		    response.result.data.forEach((line) => {
>> +			fileContent += `${line.t}\n`;
>> +		    });
>> +
>> +		    callback(null, fileContent);
>> +		},
>> +	    });
>> +	};
>> +
>> +	downloadBtn.handler = function() {
> why not defined the handler directly when instantiating above, would improve code locality
> a bit.
>
>> +	    downloadLogFull((errStatus, fileContent) => {
>> +		if (errStatus) {
>> +		    Ext.Msg.alert(gettext('Error'), errStatus);
>> +		} else {
>> +		    let record = statstore.getRecord().data;
>> +		    let fileName = record.user + "@" + record.node + "_" +
>> +			record.type + "_" + record.pid + "_" +
>> +			Proxmox.Utils.render_timestamp(record.starttime) +
>> +			"_" + record.exitstatus + ".log";
> note that in JavaScript we go for 100 characters text-width. Also, template-strings can
> make a mix of string literals plus variables often shorter or at least easier to read:
>
> let fileName = `${rec.user}@${rec.node}-${rec.type}...`
>
> albeit I'd maybe just go for the upid as is, at least avoid encoding non-identifying info
> like exit status in the file name.

Alright, I read in the developer documentation that line width should 
not exceed 80 characters
apart from some special cases. I thought that going for 100 chars 
wouldn't increase read-
ability by enough here.

So, can I safely make statements up to 100 chars or do you mean to just 
change it here?

I didn't want to use the UPID as a filename because I think it is not 
very human friendly and
can be quite hard to identify, especially if there are multiple tasklog 
files in the same folder.
The main reason for the usage here is because I wanted to include a 
human readable way
of telling the date and time in the filename (as opposed to UNIX seconds 
in the UPID)

>> +
>> +		    Proxmox.Utils.textToFile(fileName, fileContent);
>> +		}
>> +	    });
>> +	};
>> +
>> +	copyBtn.handler = function() {
> why not defined the handler directly when instantiating above, would improve code locality
> a bit.

Declaring the handlers down here was actually a deliberate decision as 
these "3 parts" of the
code above all depend on each other. Going from bottom to top:

1 - _downloadLogFull()_ function needs _let logView_ to be declared.
2 - _let logView_ needs the _let copyBtn_ and _let downloadBtn_ to be 
declared
3 - The button handlers need function _downloadLogFull()_ to be declared

In the button declaration, the handlers would be declared before the 
function is. Therefore
I moved the declaration of the handlers below the function declaration 
as to break this chain.

I actually included a comment for this but have accidentally deleted it.


Also, thanks for the review! I will adjust things for v2

>> +	    downloadLogFull((errStatus, response) => {
>> +		if (errStatus) {
>> +		    Ext.Msg.alert(gettext('Error'), errStatus);
>> +		} else {
>> +		    navigator.clipboard.writeText(response)
>> +		    .catch((err) => {
>> +			Ext.Msg.alert(gettext('Error'), err);
>> +		    });
>> +		}
>> +	    });
>> +	};
>> +
>>   	me.mon(statstore, 'load', function() {
>>   	    let status = statgrid.getObjectValue('status');
>>   
>> @@ -248,6 +310,7 @@ Ext.define('Proxmox.window.TaskViewer', {
>>   
>>   	    stop_btn1.setDisabled(status !== 'running');
>>   	    stop_btn2.setDisabled(status !== 'running');
>> +	    downloadBtn.setDisabled(status === 'running');
>>   	});
>>   
>>   	statstore.startUpdate();
From t.lamprecht@proxmox.com  Mon Apr  4 12:02:13 2022
Return-Path: <t.lamprecht@proxmox.com>
X-Original-To: pve-devel@lists.proxmox.com
Delivered-To: pve-devel@lists.proxmox.com
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by lists.proxmox.com (Postfix) with ESMTPS id 351F0A089
 for <pve-devel@lists.proxmox.com>; Mon,  4 Apr 2022 12:02:13 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id 33386B7F7
 for <pve-devel@lists.proxmox.com>; Mon,  4 Apr 2022 12:02:13 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
 [94.136.29.106])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by firstgate.proxmox.com (Proxmox) with ESMTPS id 79066B7EE
 for <pve-devel@lists.proxmox.com>; Mon,  4 Apr 2022 12:02:12 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
 by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 5132B4586A
 for <pve-devel@lists.proxmox.com>; Mon,  4 Apr 2022 12:02:12 +0200 (CEST)
Message-ID: <2a4863fa-2d27-f088-f381-09e02f9180c4@proxmox.com>
Date: Mon, 4 Apr 2022 12:01:55 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:99.0) Gecko/20100101
 Thunderbird/99.0
Content-Language: en-US
To: Daniel Tschlatscher <d.tschlatscher@proxmox.com>,
 Proxmox VE development discussion <pve-devel@lists.proxmox.com>
References: <20220401140758.1997754-1-d.tschlatscher@proxmox.com>
 <133ace32-ee00-e923-30b4-f33747dc3f1c@proxmox.com>
 <0cdb2378-9ca4-9348-303c-bab46acbc1d6@proxmox.com>
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
In-Reply-To: <0cdb2378-9ca4-9348-303c-bab46acbc1d6@proxmox.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.370 Adjusted score from AWL reputation of From: address
 BAYES_00                 -1.9 Bayes spam probability is 0 to 1%
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 NICE_REPLY_A           -0.631 Looks like a legit reply (A)
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
 T_SCC_BODY_TEXT_LINE    -0.01 -
Subject: Re: [pve-devel] [PATCH widget-toolkit] fix #3971: Download and copy
 button in the TaskViewer
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Mon, 04 Apr 2022 10:02:13 -0000

On 04.04.22 11:25, Daniel Tschlatscher wrote:
>> note=C2=A0that=C2=A0in=C2=A0JavaScript=C2=A0we=C2=A0go=C2=A0for=C2=A01=
00=C2=A0characters=C2=A0text-width.=C2=A0Also,=C2=A0template-strings=C2=A0=
can
>> make=C2=A0a=C2=A0mix=C2=A0of=C2=A0string=C2=A0literals=C2=A0plus=C2=A0=
variables=C2=A0often=C2=A0shorter=C2=A0or=C2=A0at=C2=A0least=C2=A0easier=C2=
=A0to=C2=A0read:
>>
>> let=C2=A0fileName=C2=A0=3D=C2=A0`${rec.user}@${rec.node}-${rec.type}..=
=2E`
>>
>> albeit=C2=A0I'd=C2=A0maybe=C2=A0just=C2=A0go=C2=A0for=C2=A0the=C2=A0up=
id=C2=A0as=C2=A0is,=C2=A0at=C2=A0least=C2=A0avoid=C2=A0encoding=C2=A0non-=
identifying=C2=A0info
>> like=C2=A0exit=C2=A0status=C2=A0in=C2=A0the=C2=A0file=C2=A0name.
>=20
> Alright, I read in the developer documentation that line width should n=
ot=C2=A0exceed=C2=A080=C2=A0characters
> apart from some special cases. I thought that going for 100 chars would=
n't=C2=A0increase=C2=A0read-
> ability=C2=A0by=C2=A0enough=C2=A0here.

I mean, you also did not really maxed out 80cc either FWICT ;-) But anyho=
w, thanks for the
pointer about the outdated style guides, I updated those in that regard.

>=20
> So, can I safely make statements up to 100 chars or do you mean to just=
 change=C2=A0it=C2=A0here?
>=20

yes, please do.

> I didn't want to use the UPID as a filename because I think it is not v=
ery=C2=A0human=C2=A0friendly=C2=A0and
> can be quite hard to identify, especially if there are multiple tasklog=
 files=C2=A0in=C2=A0the=C2=A0same=C2=A0folder.
> The main reason for the usage here is because I wanted to include a hum=
an=C2=A0readable=C2=A0way
> of telling the date and time in the filename (as opposed to UNIX second=
s in=C2=A0the=C2=A0UPID)

The UPID is used in many places and isn't included in the task log itself=
, so loosing that
single full ID of a task can make cross-correlation harder.

So lets please included it, if you want to have humand readable info in a=
ddition I'd go for
something like (pseudo code): `${taskUPID}-${taskStartDateISO8601}.log`

>=20
>>> +
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0Proxmox.Utils.textToFile(fileName,=C2=A0fileContent);
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0});
>>> +=C2=A0=C2=A0=C2=A0=C2=A0};
>>> +
>>> +=C2=A0=C2=A0=C2=A0=C2=A0copyBtn.handler=C2=A0=3D=C2=A0function()=C2=A0=
{
>> why=C2=A0not=C2=A0defined=C2=A0the=C2=A0handler=C2=A0directly=C2=A0whe=
n=C2=A0instantiating=C2=A0above,=C2=A0would=C2=A0improve=C2=A0code=C2=A0l=
ocality
>> a=C2=A0bit.
>=20
> Declaring the handlers down here was actually a deliberate decision as =
these=C2=A0"3=C2=A0parts"=C2=A0of=C2=A0the
> code=C2=A0above=C2=A0all=C2=A0depend=C2=A0on=C2=A0each=C2=A0other.=C2=A0=
Going=C2=A0from=C2=A0bottom=C2=A0to=C2=A0top:
>=20
> 1=C2=A0-=C2=A0_downloadLogFull()_=C2=A0function=C2=A0needs=C2=A0_let=C2=
=A0logView_=C2=A0to=C2=A0be=C2=A0declared.
> 2 - _let logView_ needs the _let copyBtn_ and _let downloadBtn_ to be d=
eclared
> 3=C2=A0-=C2=A0The=C2=A0button=C2=A0handlers=C2=A0need=C2=A0function=C2=A0=
_downloadLogFull()_=C2=A0to=C2=A0be=C2=A0declared

I'd see such needs resulting from tight coupling rather as a sign of code=
 smell.
And I only saw the direct access to private parts of the logView's viewMo=
del now
that you explicitly mentioned 1.), lets please don't do that, _if_ we req=
uire the
access to the data then lets go overt the actual public interfaces, reque=
sting the
viewModel from logView and then the data via viewModel's get(key) method.=


But actually taking a step back and questioning the reason for being requ=
ired to do
this: We really don't care about limit, we just want all - so what we act=
ually would
like to have is telling the backend to give us everything via passing `li=
mit =3D 0`
explicitly. That requires trivial changes in PVE::Tools::dump_logfile but=
 would make
for a cleaner approach.

Besides from that, logs can get huge your current approach copies everyth=
ing in memory,
which may not be returned to the OS immediately as perl normally keeps al=
located memory.
While its mainly for reuse, huge and/or parallel-triggered allocations ca=
n still make
a big memory impact (easy DOS target for users with only very basic privs=
=2E)
So, for the limit=3D=3D0 "give me everything" case and no filter set in t=
he task log api we
may not even want to call the dump_logfile but just open the file and ret=
urn in to the
webserver so that it can stream it efficiently, directly from the file.

We recently switched over the syslog/journal api call to that for similar=
 reasons.

long story short, that not only would make this all much more efficient, =
it'd also avoid
some ugly coupling.