From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>,
Dominik Csapak <d.csapak@proxmox.com>
Subject: Re: [pbs-devel] [PATCH proxmox-backup v7 8/8] ui: add MetricServerView and use it
Date: Tue, 7 Jun 2022 14:55:28 +0200 [thread overview]
Message-ID: <b42b32fa-8c6e-6e98-fdd6-dd5d38f700f9@proxmox.com> (raw)
In-Reply-To: <20220602121811.3472729-9-d.csapak@proxmox.com>
Am 02/06/2022 um 14:18 schrieb Dominik Csapak:
> simple CRUD interface to show/add/edit/delete metric servers
>
> it's a bit different from PVE's so that it's harder to reuse that to
> copy it. If we need it again, we can still refactor and combine them.
>
looks OK, just some code style nits inline.
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> www/Makefile | 1 +
> www/NavigationTree.js | 6 ++
> www/config/MetricServerView.js | 145 +++++++++++++++++++++++++++++++++
> 3 files changed, 152 insertions(+)
> create mode 100644 www/config/MetricServerView.js
>
> diff --git a/www/Makefile b/www/Makefile
> index 3a36daba..757c1fd5 100644
> --- a/www/Makefile
> +++ b/www/Makefile
> @@ -62,6 +62,7 @@ JSSRC= \
> config/WebauthnView.js \
> config/CertificateView.js \
> config/NodeOptionView.js \
> + config/MetricServerView.js \
> window/ACLEdit.js \
> window/BackupFileDownloader.js \
> window/BackupGroupChangeOwner.js \
> diff --git a/www/NavigationTree.js b/www/NavigationTree.js
> index 5f44aace..dc69f312 100644
> --- a/www/NavigationTree.js
> +++ b/www/NavigationTree.js
> @@ -68,6 +68,12 @@ Ext.define('PBS.store.NavigationStore', {
> path: 'pbsCertificateConfiguration',
> leaf: true,
> },
> + {
> + text: gettext('Metric Server'),
> + iconCls: 'fa fa-bar-chart',
> + path: 'pbsMetricServerView',
> + leaf: true,
> + },
I'd rather put this as tab in the general "Configuration" panel, this is slightly too
niche to take up space in the main navigation tree directly.
> {
> text: gettext('Subscription'),
> iconCls: 'fa fa-support',
> diff --git a/www/config/MetricServerView.js b/www/config/MetricServerView.js
> new file mode 100644
> index 00000000..b904a427
> --- /dev/null
> +++ b/www/config/MetricServerView.js
> @@ -0,0 +1,145 @@
> +Ext.define('PBS.config.MetricServerView', {
> + extend: 'Ext.grid.Panel',
> + alias: ['widget.pbsMetricServerView'],
> +
> + stateful: true,
> + stateId: 'grid-metricserver',
> +
> + controller: {
> + xclass: 'Ext.app.ViewController',
> +
> + render_type: function(value) {
I'd prefer camelCase for new functions where sensible, like ExtJS uses everywhere and part of
our code base also does.
> + switch (value) {
> + case 'influxdb-http': return "InfluxDB (HTTP)";
> + case 'influxdb-udp': return "InfluxDB (UDP)";
> + default: return Proxmox.Utils.unknownText;
> + }
I'd like to avoid switch case in JS, at least most of the time, there seldom nice and either
use more space while still being more noisy/harder to read than objects or require having
multiple statements on a single line, both not to nice. If JS would have a match statement
allowing to return a expression like we got in rust I'd be sold though.
Rather just use a object. with multiple such similar metric type derived info one could also
use a nested, schema like one
{
indlufxdb: {
xtype: '...',
name: '...',
},
}
While this is also using quite some vertical space it's at least easy to read due to being
like a static schema definition.
The renderer could then be also just inline, e.g.:
v => PBS.Schema.metricServer[v]?.name ?? Proxmox.Utils.unknownText,
but not to hard feelings for that.
> + },
> +
> + get_xtype: function(value) {
> + switch (value) {
same here (and thus the schema proposal)
> + case 'influxdb-http': return "InfluxDbHttp";
> + case 'influxdb-udp': return "InfluxDbUdp";
> + default: throw "invalid type";
> + }
> + },
> +
> + editWindow: function(xtype, id) {
> + let me = this;
> + Ext.create(`PBS.window.${xtype}Edit`, {
> + serverid: id,
> + autoShow: true,
> + autoLoad: true,
> + listeners: {
> + destroy: () => me.reload(),
> + },
> + });
> + },
> +
> + addServer: function(button) {
> + this.editWindow(this.get_xtype(button.type));
> + },
> +
> + editServer: function() {
> + let me = this;
> + let view = me.getView();
> + let selection = view.getSelection();
> + if (!selection || selection.length < 1) {
> + return;
> + }
> +
> + let cfg = selection[0].data;
> +
> + let xtype = me.get_xtype(cfg.type);
> + me.editWindow(xtype, cfg.name);
> + },
> +
> + reload: function() {
> + this.getView().getStore().load();
> + },
> + },
> +
> + store: {
> + autoLoad: true,
> + id: 'metricservers',
> + proxy: {
> + type: 'proxmox',
> + url: '/api2/json/admin/metricserver',
> + },
> + },
> +
> + columns: [
> + {
> + text: gettext('Name'),
> + flex: 2,
> + dataIndex: 'name',
> + },
> + {
> + text: gettext('Type'),
> + width: 150,
> + dataIndex: 'type',
> + renderer: 'render_type',
> + },
> + {
> + text: gettext('Enabled'),
> + dataIndex: 'disable',
> + width: 100,
> + renderer: Proxmox.Utils.format_neg_boolean,
> + },
> + {
> + text: gettext('Target Server'),
> + width: 200,
> + dataIndex: 'server',
> + },
> + {
> + text: gettext('Comment'),
> + flex: 3,
> + dataIndex: 'comment',
> + renderer: Ext.htmlEncode,
> + },
> + ],
> +
> + tbar: [
> + {
> + text: gettext('Add'),
> + menu: [
> + {
> + text: 'InfluxDB (HTTP)',
> + type: 'influxdb-http',
> + iconCls: 'fa fa-fw fa-bar-chart',
> + handler: 'addServer',
> + },
> + {
> + text: 'InfluxDB (UDP)',
> + type: 'influxdb-udp',
> + iconCls: 'fa fa-fw fa-bar-chart',
> + handler: 'addServer',
> + },
> + ],
> + },
> + {
> + text: gettext('Edit'),
> + xtype: 'proxmoxButton',
> + handler: 'editServer',
> + disabled: true,
> + },
> + {
> + xtype: 'proxmoxStdRemoveButton',
> + getUrl: (rec) => `/api2/extjs/config/metricserver/${rec.data.type}/${rec.data.name}`,
> + getRecordName: (rec) => rec.data.name,
> + callback: 'reload',
> + },
> + ],
> +
> + listeners: {
> + itemdblclick: 'editServer',
> + },
> +
> + initComponent: function() {
> + var me = this;
> +
> + me.callParent();
> +
> + Proxmox.Utils.monStoreErrors(me, me.getStore());
> + },
> +});
prev parent reply other threads:[~2022-06-07 12:55 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-02 12:18 [pbs-devel] [PATCH proxmox-backup v7 0/8] add metrics server capability Dominik Csapak
2022-06-02 12:18 ` [pbs-devel] [PATCH proxmox-backup v7 1/8] use 'fs_info' from proxmox-sys Dominik Csapak
2022-06-07 6:51 ` [pbs-devel] applied: " Thomas Lamprecht
2022-06-02 12:18 ` [pbs-devel] [PATCH proxmox-backup v7 2/8] pbs-api-types: add metrics api types Dominik Csapak
2022-06-02 12:18 ` [pbs-devel] [PATCH proxmox-backup v7 3/8] pbs-config: add metrics config class Dominik Csapak
2022-06-03 10:16 ` Wolfgang Bumiller
2022-06-02 12:18 ` [pbs-devel] [PATCH proxmox-backup v7 4/8] backup-proxy: decouple stats gathering from rrd update Dominik Csapak
2022-06-02 14:09 ` Matthias Heiserer
2022-06-02 12:18 ` [pbs-devel] [PATCH proxmox-backup v7 5/8] proxmox-backup-proxy: send metrics to configured metrics server Dominik Csapak
2022-06-02 12:18 ` [pbs-devel] [PATCH proxmox-backup v7 6/8] api: add metricserver endpoints Dominik Csapak
2022-06-07 12:44 ` Thomas Lamprecht
2022-06-08 8:23 ` Dominik Csapak
2022-06-02 12:18 ` [pbs-devel] [PATCH proxmox-backup v7 7/8] ui: add window/InfluxDbEdit Dominik Csapak
2022-06-02 12:18 ` [pbs-devel] [PATCH proxmox-backup v7 8/8] ui: add MetricServerView and use it Dominik Csapak
2022-06-07 12:55 ` Thomas Lamprecht [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b42b32fa-8c6e-6e98-fdd6-dd5d38f700f9@proxmox.com \
--to=t.lamprecht@proxmox.com \
--cc=d.csapak@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox