From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 C1317A065 for ; Mon, 4 Apr 2022 13:59:18 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id AFC19C573 for ; Mon, 4 Apr 2022 13:58:48 +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 EACD7C56A for ; Mon, 4 Apr 2022 13:58:47 +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 BD05841F35 for ; Mon, 4 Apr 2022 13:58:47 +0200 (CEST) Message-ID: <89d0e963-fe40-915b-d798-17f3801455ae@proxmox.com> Date: Mon, 4 Apr 2022 13:58:30 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Content-Language: en-US To: Thomas Lamprecht , Proxmox VE development discussion References: <20220401140758.1997754-1-d.tschlatscher@proxmox.com> <133ace32-ee00-e923-30b4-f33747dc3f1c@proxmox.com> <0cdb2378-9ca4-9348-303c-bab46acbc1d6@proxmox.com> <2a4863fa-2d27-f088-f381-09e02f9180c4@proxmox.com> From: Daniel Tschlatscher In-Reply-To: <2a4863fa-2d27-f088-f381-09e02f9180c4@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.439 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Apr 2022 11:59:18 -0000 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. >