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

[Thomas Lamprecht]

On 04.04.22 11:25, Daniel Tschlatscher wrote:
>> 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.

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

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

yes, please do.

> 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)

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 addition I'd go for
something like (pseudo code): `${taskUPID}-${taskStartDateISO8601}.log`

> 
>>> +
>>> +            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

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 viewModel now
that you explicitly mentioned 1.), lets please don't do that, _if_ we require the
access to the data then lets go overt the actual public interfaces, requesting 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 required to do
this: We really don't care about limit, we just want all - so what we actually would
like to have is telling the backend to give us everything via passing `limit = 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 everything in memory,
which may not be returned to the OS immediately as perl normally keeps allocated memory.
While its mainly for reuse, huge and/or parallel-triggered allocations can still make
a big memory impact (easy DOS target for users with only very basic privs.)
So, for the limit==0 "give me everything" case and no filter set in the task log api we
may not even want to call the dump_logfile but just open the file and return 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.