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

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

On 4/4/22 12:01, Thomas Lamprecht wrote:
> 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.
Could you give me a pointer to where I can find such an implementation 
for syslog and journal?
I looked up the API function declarations and the dump_xxx functions but 
I could not find an example
where the server would actually stream and return a file when called 
with limit undefined or 0.
Am I looking in the wrong repositories or did I misunderstand the 
sentence above?
>
> long story short, that not only would make this all much more efficient, it'd also avoid
> some ugly coupling.
>