[pbs-devel] [RFC PATCH proxmox-backup 0/3] show upload statistics in gui

[pbs-devel] [RFC PATCH proxmox-backup 0/3] show upload statistics in gui

[Dominik Csapak]

Sending as RFC because:
* not sure if we want to increase the size of the 'list snapshots' api
  call even further (albeit by a small amount per snapshot that we
  already have anyway in memory)
* not sure if we want to have another action in the gui

alternatively we could:
* add a seperate api call to get detailed info about a snapshot
  (incl the upload statistics)
* add the info as a tooltip to the 'size' column (or somewhere else?)
  i just fear that this is too subtle for most people to notice

Dominik Csapak (3):
  api-types: make UploadStatistic an api type
  api: datastore: add upload_statistic to snapshot listing
  ui: datastore content: add action to show upload statistics

 pbs-api-types/src/datastore.rs | 41 +++++++++++++++++++++++++
 src/api2/admin/datastore.rs    | 22 ++++++++++---
 src/api2/backup/environment.rs | 35 +--------------------
 www/datastore/Content.js       | 56 ++++++++++++++++++++++++++++++++++
 4 files changed, 115 insertions(+), 39 deletions(-)

-- 
2.30.2

[pbs-devel] [RFC PATCH proxmox-backup 1/3] api-types: make UploadStatistic an api type

[Dominik Csapak]

and move it to pbs_api_types. We'll want this to expose on the api.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pbs-api-types/src/datastore.rs | 38 ++++++++++++++++++++++++++++++++++
 src/api2/backup/environment.rs | 35 +------------------------------
 2 files changed, 39 insertions(+), 34 deletions(-)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 73c4890e..41db680c 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -419,6 +419,44 @@ pub struct SnapshotVerifyState {
     pub state: VerifyState,
 }
 
+#[api()]
+#[derive(Copy, Clone, Serialize, Deserialize)]
+/// Chunk upload statistics of a snapshot
+pub struct UploadStatistic {
+    /// Amount of chunks uploaded (incl. duplicates)
+    pub count: u64,
+    /// Uncompressed bytes uploaded
+    pub size: u64,
+    /// Compressed bytes uploaded
+    pub compressed_size: u64,
+    /// Amount of duplicate chunks uploaded
+    pub duplicates: u64,
+}
+
+impl UploadStatistic {
+    pub fn new() -> Self {
+        Self {
+            count: 0,
+            size: 0,
+            compressed_size: 0,
+            duplicates: 0,
+        }
+    }
+}
+
+impl std::ops::Add for UploadStatistic {
+    type Output = Self;
+
+    fn add(self, other: Self) -> Self {
+        Self {
+            count: self.count + other.count,
+            size: self.size + other.size,
+            compressed_size: self.compressed_size + other.compressed_size,
+            duplicates: self.duplicates + other.duplicates,
+        }
+    }
+}
+
 /// A namespace provides a logical separation between backup groups from different domains
 /// (cluster, sites, ...) where uniqueness cannot be guaranteed anymore. It allows users to share a
 /// datastore (i.e., one deduplication domain (chunk store)) with multiple (trusted) sites and
diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
index 5291bce8..0188bffc 100644
--- a/src/api2/backup/environment.rs
+++ b/src/api2/backup/environment.rs
@@ -3,13 +3,12 @@ use nix::dir::Dir;
 use std::collections::HashMap;
 use std::sync::{Arc, Mutex};
 
-use ::serde::Serialize;
 use serde_json::{json, Value};
 
 use proxmox_router::{RpcEnvironment, RpcEnvironmentType};
 use proxmox_sys::fs::{lock_dir_noblock_shared, replace_file, CreateOptions};
 
-use pbs_api_types::Authid;
+use pbs_api_types::{Authid, UploadStatistic};
 use pbs_datastore::backup_info::{BackupDir, BackupInfo};
 use pbs_datastore::dynamic_index::DynamicIndexWriter;
 use pbs_datastore::fixed_index::FixedIndexWriter;
@@ -20,38 +19,6 @@ use crate::backup::verify_backup_dir_with_lock;
 
 use hyper::{Body, Response};
 
-#[derive(Copy, Clone, Serialize)]
-struct UploadStatistic {
-    count: u64,
-    size: u64,
-    compressed_size: u64,
-    duplicates: u64,
-}
-
-impl UploadStatistic {
-    fn new() -> Self {
-        Self {
-            count: 0,
-            size: 0,
-            compressed_size: 0,
-            duplicates: 0,
-        }
-    }
-}
-
-impl std::ops::Add for UploadStatistic {
-    type Output = Self;
-
-    fn add(self, other: Self) -> Self {
-        Self {
-            count: self.count + other.count,
-            size: self.size + other.size,
-            compressed_size: self.compressed_size + other.compressed_size,
-            duplicates: self.duplicates + other.duplicates,
-        }
-    }
-}
-
 struct DynamicWriterState {
     name: String,
     index: DynamicIndexWriter,
-- 
2.30.2

[pbs-devel] [RFC PATCH proxmox-backup 2/3] api: datastore: add upload_statistic to snapshot listing

[Dominik Csapak]

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
instead we could make a seperate api call that return just detailed info
about a single snapshot on demand
 pbs-api-types/src/datastore.rs |  3 +++
 src/api2/admin/datastore.rs    | 22 +++++++++++++++++-----
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 41db680c..39e3c09a 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -1138,6 +1138,9 @@ pub struct SnapshotListItem {
     /// Protection from prunes
     #[serde(default)]
     pub protected: bool,
+    /// Upload statistics
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub upload_statistic: Option<UploadStatistic>,
 }
 
 #[api(
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index a95031e7..f5b47cf4 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -35,11 +35,11 @@ use pbs_api_types::{
     print_ns_and_snapshot, print_store_and_ns, Authid, BackupContent, BackupNamespace, BackupType,
     Counts, CryptMode, DataStoreListItem, DataStoreStatus, GarbageCollectionStatus, GroupListItem,
     KeepOptions, Operation, PruneJobOptions, RRDMode, RRDTimeFrame, SnapshotListItem,
-    SnapshotVerifyState, BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA,
-    BACKUP_TIME_SCHEMA, BACKUP_TYPE_SCHEMA, DATASTORE_SCHEMA, IGNORE_VERIFIED_BACKUPS_SCHEMA,
-    MAX_NAMESPACE_DEPTH, NS_MAX_DEPTH_SCHEMA, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
-    PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_DATASTORE_READ, PRIV_DATASTORE_VERIFY,
-    UPID_SCHEMA, VERIFICATION_OUTDATED_AFTER_SCHEMA,
+    SnapshotVerifyState, UploadStatistic, BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA,
+    BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA, BACKUP_TYPE_SCHEMA, DATASTORE_SCHEMA,
+    IGNORE_VERIFIED_BACKUPS_SCHEMA, MAX_NAMESPACE_DEPTH, NS_MAX_DEPTH_SCHEMA, PRIV_DATASTORE_AUDIT,
+    PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_DATASTORE_READ,
+    PRIV_DATASTORE_VERIFY, UPID_SCHEMA, VERIFICATION_OUTDATED_AFTER_SCHEMA,
 };
 use pbs_client::pxar::{create_tar, create_zip};
 use pbs_config::CachedUserInfo;
@@ -532,6 +532,16 @@ unsafe fn list_snapshots_blocking(
 
                 let size = Some(files.iter().map(|x| x.size.unwrap_or(0)).sum());
 
+                let upload_statistic = manifest.unprotected["chunk_upload_stats"].clone();
+                let upload_statistic: Option<UploadStatistic> =
+                    match serde_json::from_value(upload_statistic) {
+                        Ok(stats) => stats,
+                        Err(err) => {
+                            log::warn!("error parsing chunk_upload_stats: {err}");
+                            None
+                        }
+                    };
+
                 SnapshotListItem {
                     backup,
                     comment,
@@ -541,6 +551,7 @@ unsafe fn list_snapshots_blocking(
                     size,
                     owner,
                     protected,
+                    upload_statistic,
                 }
             }
             Err(err) => {
@@ -564,6 +575,7 @@ unsafe fn list_snapshots_blocking(
                     size: None,
                     owner,
                     protected,
+                    upload_statistic: None,
                 }
             }
         }
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 3/3] ui: datastore content: add action to show upload statistics

[Dominik Csapak]

inspired by how we show volume statistics for tape drives

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
we could also add it as a tooltip somewhere else, eg the size column

also, this pattern for the window could be refactored into a
'keyvalueinfowindow' (or something like that), since we already use that
pattern a few time

 www/datastore/Content.js | 56 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/www/datastore/Content.js b/www/datastore/Content.js
index 9fc07d49..bb2d76ee 100644
--- a/www/datastore/Content.js
+++ b/www/datastore/Content.js
@@ -711,6 +711,56 @@ Ext.define('PBS.DataStoreContent', {
 	    });
 	},
 
+	showUploadStatistics: function(view, rI, cI, item, e, rec) {
+	    let me = this;
+
+	    let list = [];
+
+	    let keyMap = {
+		count: gettext('Chunk Count'),
+		duplicates: gettext('Duplicate Chunks'),
+		size: gettext('Size'),
+		compressed_size: gettext('Compressed Size'),
+	    };
+
+	    for (let [key, val] of Object.entries(rec.data['upload-statistic'])) {
+		if (key === 'size' || key === 'compressed_size') {
+		    val = Proxmox.Utils.format_size(val);
+		}
+
+		list.push({ key: keyMap[key] ?? key, value: val });
+	    }
+
+	    Ext.create('Ext.window.Window', {
+		title: gettext('Upload Statistics'),
+		modal: true,
+		width: 600,
+		height: 250,
+		layout: 'fit',
+		scrollable: true,
+		items: [
+		    {
+			xtype: 'grid',
+			store: {
+			    data: list,
+			},
+			columns: [
+			    {
+				text: gettext('Property'),
+				dataIndex: 'key',
+				flex: 1,
+			    },
+			    {
+				text: gettext('Value'),
+				dataIndex: 'value',
+				flex: 1,
+			    },
+			],
+		    },
+		],
+	    }).show();
+	},
+
 	onForget: function(table, rI, cI, item, e, { data }) {
 	    let me = this;
 	    let view = this.getView();
@@ -974,6 +1024,12 @@ Ext.define('PBS.DataStoreContent', {
 		    },
 		    isActionDisabled: (v, r, c, i, rec) => rec.data.ty !== 'dir',
 		},
+		{
+		    handler: 'showUploadStatistics',
+		    getTip: (v, m, rec) => Ext.String.format(gettext("Show Upload Statistics of '{0}'"), v),
+		    getClass: (v, m, rec) => rec.data.ty === 'dir' ? 'fa fa-info-circle' : 'pmx-hidden',
+		    isActionDisabled: (v, r, c, i, rec) => rec.data.ty !== 'dir',
+		},
 		{
 		    handler: 'onForget',
 		    getTip: (v, m, { data }) => {
-- 
2.30.2

Re: [pbs-devel] [RFC PATCH proxmox-backup 2/3] api: datastore: add upload_statistic to snapshot listing

[Wolfgang Bumiller]

On Tue, Aug 01, 2023 at 11:29:53AM +0200, Dominik Csapak wrote:
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> instead we could make a seperate api call that return just detailed info
> about a single snapshot on demand
>  pbs-api-types/src/datastore.rs |  3 +++
>  src/api2/admin/datastore.rs    | 22 +++++++++++++++++-----
>  2 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> index 41db680c..39e3c09a 100644
> --- a/pbs-api-types/src/datastore.rs
> +++ b/pbs-api-types/src/datastore.rs
> @@ -1138,6 +1138,9 @@ pub struct SnapshotListItem {
>      /// Protection from prunes
>      #[serde(default)]
>      pub protected: bool,
> +    /// Upload statistics
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub upload_statistic: Option<UploadStatistic>,

^ SnapshotListItem has since been made PartialEq, so UploadStatistic
also needs to derive it.

>  }
>  
>  #[api(
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index a95031e7..f5b47cf4 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -35,11 +35,11 @@ use pbs_api_types::{
>      print_ns_and_snapshot, print_store_and_ns, Authid, BackupContent, BackupNamespace, BackupType,
>      Counts, CryptMode, DataStoreListItem, DataStoreStatus, GarbageCollectionStatus, GroupListItem,
>      KeepOptions, Operation, PruneJobOptions, RRDMode, RRDTimeFrame, SnapshotListItem,
> -    SnapshotVerifyState, BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA,
> -    BACKUP_TIME_SCHEMA, BACKUP_TYPE_SCHEMA, DATASTORE_SCHEMA, IGNORE_VERIFIED_BACKUPS_SCHEMA,
> -    MAX_NAMESPACE_DEPTH, NS_MAX_DEPTH_SCHEMA, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
> -    PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_DATASTORE_READ, PRIV_DATASTORE_VERIFY,
> -    UPID_SCHEMA, VERIFICATION_OUTDATED_AFTER_SCHEMA,
> +    SnapshotVerifyState, UploadStatistic, BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA,
> +    BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA, BACKUP_TYPE_SCHEMA, DATASTORE_SCHEMA,
> +    IGNORE_VERIFIED_BACKUPS_SCHEMA, MAX_NAMESPACE_DEPTH, NS_MAX_DEPTH_SCHEMA, PRIV_DATASTORE_AUDIT,
> +    PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_DATASTORE_READ,
> +    PRIV_DATASTORE_VERIFY, UPID_SCHEMA, VERIFICATION_OUTDATED_AFTER_SCHEMA,
>  };
>  use pbs_client::pxar::{create_tar, create_zip};
>  use pbs_config::CachedUserInfo;
> @@ -532,6 +532,16 @@ unsafe fn list_snapshots_blocking(
>  
>                  let size = Some(files.iter().map(|x| x.size.unwrap_or(0)).sum());
>  
> +                let upload_statistic = manifest.unprotected["chunk_upload_stats"].clone();
> +                let upload_statistic: Option<UploadStatistic> =

Note that `Value` as well as `&Value` are also both Deserialize*R*s,
so you can skip cloning the Value structure and go straight to the
`UploadStatistic` via:

    let upload_statistic = match Option::<UploadStatistic>::deserialize(
        &manifest.unprotected["chunk_upload_stats"],
    ) {
        ...
    }

Re: [pbs-devel] [PATCH proxmox-backup 3/3] ui: datastore content: add action to show upload statistics

[Thomas Lamprecht]

On 01.08.23 11:29, Dominik Csapak wrote:
> inspired by how we show volume statistics for tape drives
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> we could also add it as a tooltip somewhere else, eg the size column
> 
> also, this pattern for the window could be refactored into a
> 'keyvalueinfowindow' (or something like that), since we already use that
> pattern a few time
> 
>  www/datastore/Content.js | 56 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/www/datastore/Content.js b/www/datastore/Content.js
> index 9fc07d49..bb2d76ee 100644
> --- a/www/datastore/Content.js
> +++ b/www/datastore/Content.js
> @@ -711,6 +711,56 @@ Ext.define('PBS.DataStoreContent', {
>  	    });
>  	},
>  
> +	showUploadStatistics: function(view, rI, cI, item, e, rec) {
> +	    let me = this;
> +
> +	    let list = [];
> +
> +	    let keyMap = {
> +		count: gettext('Chunk Count'),
> +		duplicates: gettext('Duplicate Chunks'),
> +		size: gettext('Size'),
> +		compressed_size: gettext('Compressed Size'),
> +	    };
> +
> +	    for (let [key, val] of Object.entries(rec.data['upload-statistic'])) {

should we explicitly sort those? (e.g., by a predefined weight)

> +		if (key === 'size' || key === 'compressed_size') {
> +		    val = Proxmox.Utils.format_size(val);
> +		}

why mix a declarative map and some ad-hoc if checks?

Rather reuse the map above for a renderer:

    let schema = {
	count: {
	    label: gettext('Chunk Count'),
        },
	duplicates: {
	    label: gettext('Duplicate Chunks'),
	},
	size: {
	    label: gettext('Size'),
	    renderer: Proxmox.Utils.format_size,
	},
	compressed_size: {
	    label: gettext('Compressed Size'),
	    renderer: Proxmox.Utils.format_size,
	},
    };

> +
> +		list.push({ key: keyMap[key] ?? key, value: val });
> +	    }
> +
> +	    Ext.create('Ext.window.Window', {
> +		title: gettext('Upload Statistics'),
> +		modal: true,
> +		width: 600,
> +		height: 250,
> +		layout: 'fit',
> +		scrollable: true,
> +		items: [
> +		    {
> +			xtype: 'grid',
> +			store: {
> +			    data: list,
> +			},
> +			columns: [
> +			    {
> +				text: gettext('Property'),
> +				dataIndex: 'key',
> +				flex: 1,
> +			    },
> +			    {
> +				text: gettext('Value'),
> +				dataIndex: 'value',
> +				flex: 1,
> +			    },

I'd not bother showing that header, basically just noise, and for
such small row count even sorting would not make that much sense.

> +			],
> +		    },
> +		],
> +	    }).show();
> +	},
> +
>  	onForget: function(table, rI, cI, item, e, { data }) {
>  	    let me = this;
>  	    let view = this.getView();
> @@ -974,6 +1024,12 @@ Ext.define('PBS.DataStoreContent', {
>  		    },
>  		    isActionDisabled: (v, r, c, i, rec) => rec.data.ty !== 'dir',
>  		},
> +		{
> +		    handler: 'showUploadStatistics',
> +		    getTip: (v, m, rec) => Ext.String.format(gettext("Show Upload Statistics of '{0}'"), v),

for tooltips it probably makes more sense to use sentence case.

> +		    getClass: (v, m, rec) => rec.data.ty === 'dir' ? 'fa fa-info-circle' : 'pmx-hidden',

info-circle is not a good icon for some specific stats, i.e., not a
general info about the backup snapshot.. A stop-watch could be nice,
but there doesn't seem to be any, so possibly "fa-file-text-o" (a
sheet of stat lines, so to say), not ideal too but IMO better than
the info-circle.

ps. maybe injecting some more general info like duration could be
nice (didn't check if we even have that available already here
though).

That said maybe one could even make this an actual info dialog,
with the stats only be one part of that, then the info-circle
could be OK too and we'd not add a core UI element for a rather
niche information that most won't look at often.

> +		    isActionDisabled: (v, r, c, i, rec) => rec.data.ty !== 'dir',
> +		},
>  		{
>  		    handler: 'onForget',
>  		    getTip: (v, m, { data }) => {

Re: [pbs-devel] [RFC PATCH proxmox-backup 1/3] api-types: make UploadStatistic an api type

[Thomas Lamprecht]

On 01.08.23 11:29, Dominik Csapak wrote:
> and move it to pbs_api_types. We'll want this to expose on the api.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  pbs-api-types/src/datastore.rs | 38 ++++++++++++++++++++++++++++++++++
>  src/api2/backup/environment.rs | 35 +------------------------------
>  2 files changed, 39 insertions(+), 34 deletions(-)
> 
> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> index 73c4890e..41db680c 100644
> --- a/pbs-api-types/src/datastore.rs
> +++ b/pbs-api-types/src/datastore.rs
> @@ -419,6 +419,44 @@ pub struct SnapshotVerifyState {
>      pub state: VerifyState,
>  }
>  
> +#[api()]
> +#[derive(Copy, Clone, Serialize, Deserialize)]

misses: 

#[serde(rename_all = "kebab-case")]

or does that break manifest?

If, then mabye a simple clone/wrapper struct for the API would be nicer,
so that those two use-cases get decoupled explicitly.

> +/// Chunk upload statistics of a snapshot
> +pub struct UploadStatistic {
> +    /// Amount of chunks uploaded (incl. duplicates)
> +    pub count: u64,
> +    /// Uncompressed bytes uploaded
> +    pub size: u64,
> +    /// Compressed bytes uploaded
> +    pub compressed_size: u64,
> +    /// Amount of duplicate chunks uploaded
> +    pub duplicates: u64,
> +}
> +

Re: [pbs-devel] [RFC PATCH proxmox-backup 1/3] api-types: make UploadStatistic an api type

[Dominik Csapak]

On 11/27/23 10:52, Thomas Lamprecht wrote:
> On 01.08.23 11:29, Dominik Csapak wrote:
>> and move it to pbs_api_types. We'll want this to expose on the api.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   pbs-api-types/src/datastore.rs | 38 ++++++++++++++++++++++++++++++++++
>>   src/api2/backup/environment.rs | 35 +------------------------------
>>   2 files changed, 39 insertions(+), 34 deletions(-)
>>
>> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
>> index 73c4890e..41db680c 100644
>> --- a/pbs-api-types/src/datastore.rs
>> +++ b/pbs-api-types/src/datastore.rs
>> @@ -419,6 +419,44 @@ pub struct SnapshotVerifyState {
>>       pub state: VerifyState,
>>   }
>>   
>> +#[api()]
>> +#[derive(Copy, Clone, Serialize, Deserialize)]
> 
> misses:
> 
> #[serde(rename_all = "kebab-case")]
> 
> or does that break manifest?

it shouldn't since we only save it in the 'unprotected' field that is a 'Value'
but i'll check

> 
> If, then mabye a simple clone/wrapper struct for the API would be nicer,
> so that those two use-cases get decoupled explicitly.
> 
>> +/// Chunk upload statistics of a snapshot
>> +pub struct UploadStatistic {
>> +    /// Amount of chunks uploaded (incl. duplicates)
>> +    pub count: u64,
>> +    /// Uncompressed bytes uploaded
>> +    pub size: u64,
>> +    /// Compressed bytes uploaded
>> +    pub compressed_size: u64,
>> +    /// Amount of duplicate chunks uploaded
>> +    pub duplicates: u64,
>> +}
>> +
> 

Re: [pbs-devel] [PATCH proxmox-backup 3/3] ui: datastore content: add action to show upload statistics

[Dominik Csapak]

On 11/27/23 10:28, Thomas Lamprecht wrote:
> On 01.08.23 11:29, Dominik Csapak wrote:
>> inspired by how we show volume statistics for tape drives
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> we could also add it as a tooltip somewhere else, eg the size column
>>
>> also, this pattern for the window could be refactored into a
>> 'keyvalueinfowindow' (or something like that), since we already use that
>> pattern a few time
>>
>>   www/datastore/Content.js | 56 ++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 56 insertions(+)
>>
>> diff --git a/www/datastore/Content.js b/www/datastore/Content.js
>> index 9fc07d49..bb2d76ee 100644
>> --- a/www/datastore/Content.js
>> +++ b/www/datastore/Content.js
>> @@ -711,6 +711,56 @@ Ext.define('PBS.DataStoreContent', {
>>   	    });
>>   	},
>>   
>> +	showUploadStatistics: function(view, rI, cI, item, e, rec) {
>> +	    let me = this;
>> +
>> +	    let list = [];
>> +
>> +	    let keyMap = {
>> +		count: gettext('Chunk Count'),
>> +		duplicates: gettext('Duplicate Chunks'),
>> +		size: gettext('Size'),
>> +		compressed_size: gettext('Compressed Size'),
>> +	    };
>> +
>> +	    for (let [key, val] of Object.entries(rec.data['upload-statistic'])) {
> 
> should we explicitly sort those? (e.g., by a predefined weight)

yes, that would be better

> 
>> +		if (key === 'size' || key === 'compressed_size') {
>> +		    val = Proxmox.Utils.format_size(val);
>> +		}
> 
> why mix a declarative map and some ad-hoc if checks?
> 
> Rather reuse the map above for a renderer:
> 
>      let schema = {
> 	count: {
> 	    label: gettext('Chunk Count'),
>          },
> 	duplicates: {
> 	    label: gettext('Duplicate Chunks'),
> 	},
> 	size: {
> 	    label: gettext('Size'),
> 	    renderer: Proxmox.Utils.format_size,
> 	},
> 	compressed_size: {
> 	    label: gettext('Compressed Size'),
> 	    renderer: Proxmox.Utils.format_size,
> 	},
>      };
> 

yes makes sense

>> +
>> +		list.push({ key: keyMap[key] ?? key, value: val });
>> +	    }
>> +
>> +	    Ext.create('Ext.window.Window', {
>> +		title: gettext('Upload Statistics'),
>> +		modal: true,
>> +		width: 600,
>> +		height: 250,
>> +		layout: 'fit',
>> +		scrollable: true,
>> +		items: [
>> +		    {
>> +			xtype: 'grid',
>> +			store: {
>> +			    data: list,
>> +			},
>> +			columns: [
>> +			    {
>> +				text: gettext('Property'),
>> +				dataIndex: 'key',
>> +				flex: 1,
>> +			    },
>> +			    {
>> +				text: gettext('Value'),
>> +				dataIndex: 'value',
>> +				flex: 1,
>> +			    },
> 
> I'd not bother showing that header, basically just noise, and for
> such small row count even sorting would not make that much sense.
> 
>> +			],
>> +		    },
>> +		],
>> +	    }).show();
>> +	},
>> +
>>   	onForget: function(table, rI, cI, item, e, { data }) {
>>   	    let me = this;
>>   	    let view = this.getView();
>> @@ -974,6 +1024,12 @@ Ext.define('PBS.DataStoreContent', {
>>   		    },
>>   		    isActionDisabled: (v, r, c, i, rec) => rec.data.ty !== 'dir',
>>   		},
>> +		{
>> +		    handler: 'showUploadStatistics',
>> +		    getTip: (v, m, rec) => Ext.String.format(gettext("Show Upload Statistics of '{0}'"), v),
> 
> for tooltips it probably makes more sense to use sentence case.
> 
>> +		    getClass: (v, m, rec) => rec.data.ty === 'dir' ? 'fa fa-info-circle' : 'pmx-hidden',
> 
> info-circle is not a good icon for some specific stats, i.e., not a
> general info about the backup snapshot.. A stop-watch could be nice,
> but there doesn't seem to be any, so possibly "fa-file-text-o" (a
> sheet of stat lines, so to say), not ideal too but IMO better than
> the info-circle.
> 
> ps. maybe injecting some more general info like duration could be
> nice (didn't check if we even have that available already here
> though).
> 
> That said maybe one could even make this an actual info dialog,
> with the stats only be one part of that, then the info-circle
> could be OK too and we'd not add a core UI element for a rather
> niche information that most won't look at often.

here we basically have only the info we have in the grid already,
but we could provide it in a nicer way maybe:

backup time, files (+sizes), last verification info (+link to task log), etc.

or did you mean we add a new api endpoint that returns more info about the snapshot
altogether? (which could also make sense)

> 
>> +		    isActionDisabled: (v, r, c, i, rec) => rec.data.ty !== 'dir',
>> +		},
>>   		{
>>   		    handler: 'onForget',
>>   		    getTip: (v, m, { data }) => {
> 

Re: [pbs-devel] [PATCH proxmox-backup 3/3] ui: datastore content: add action to show upload statistics

[Thomas Lamprecht]

On 27.11.23 10:28, Thomas Lamprecht wrote:
> we'd not add a core UI element for a rather niche information
> that most won't look at often.

re-thinking this, maybe we want to put those things into a context
menu and expose that menu with a fa-bars or fa-toggle-down icon,
in addition with allowing one to trigger it with a standard right
click context menu (I wanted to add that for datastore content UI
since quite a while already).

Re: [pbs-devel] [RFC PATCH proxmox-backup 1/3] api-types: make UploadStatistic an api type

[Thomas Lamprecht]

On 27.11.23 11:01, Dominik Csapak wrote:
> On 11/27/23 10:52, Thomas Lamprecht wrote:
>> On 01.08.23 11:29, Dominik Csapak wrote:
>>> +#[api()]
>>> +#[derive(Copy, Clone, Serialize, Deserialize)]
>>
>> misses:
>>
>> #[serde(rename_all = "kebab-case")]
>>
>> or does that break manifest?
> 
> it shouldn't since we only save it in the 'unprotected' field that is a 'Value'
> but i'll check

I did not mean breakage as in "might break signatures", but as in
backward/forward compat to any of our code/tools using that field
already (tbh. not sure from top of my head if serde json magically
falls back to field casing variants, e.g., check if foo-bar is there
if not take foo_bar, I think not, so that's why I asked - should have
said so in my first response).

Re: [pbs-devel] [PATCH proxmox-backup 3/3] ui: datastore content: add action to show upload statistics

[Dominik Csapak]

On 11/27/23 11:07, Thomas Lamprecht wrote:
> On 27.11.23 10:28, Thomas Lamprecht wrote:
>> we'd not add a core UI element for a rather niche information
>> that most won't look at often.
> 
> re-thinking this, maybe we want to put those things into a context
> menu and expose that menu with a fa-bars or fa-toggle-down icon,
> in addition with allowing one to trigger it with a standard right
> click context menu (I wanted to add that for datastore content UI
> since quite a while already).

sure, doesn't sound too complicated. i'd put the remaining actions
also there. would you then still prefer a "general info" panel
over just showing the upload statistics? (i guess so, but want
to make sure)

Re: [pbs-devel] [RFC PATCH proxmox-backup 1/3] api-types: make UploadStatistic an api type

[Dominik Csapak]

On 11/27/23 11:12, Thomas Lamprecht wrote:
> On 27.11.23 11:01, Dominik Csapak wrote:
>> On 11/27/23 10:52, Thomas Lamprecht wrote:
>>> On 01.08.23 11:29, Dominik Csapak wrote:
>>>> +#[api()]
>>>> +#[derive(Copy, Clone, Serialize, Deserialize)]
>>>
>>> misses:
>>>
>>> #[serde(rename_all = "kebab-case")]
>>>
>>> or does that break manifest?
>>
>> it shouldn't since we only save it in the 'unprotected' field that is a 'Value'
>> but i'll check
> 
> I did not mean breakage as in "might break signatures", but as in
> backward/forward compat to any of our code/tools using that field
> already (tbh. not sure from top of my head if serde json magically
> falls back to field casing variants, e.g., check if foo-bar is there
> if not take foo_bar, I think not, so that's why I asked - should have
> said so in my first response).

mhmm.. we only ever write that once (during backup finish) and never read
it anywhere (until my patch) so it couldn't break any existing code.

adding the rename now would only affect new backups, but you might be
right that deserializing older backups might not be working then.

again, i'll check

Re: [pbs-devel] [PATCH proxmox-backup 3/3] ui: datastore content: add action to show upload statistics

[Thomas Lamprecht]

On 27.11.23 11:04, Dominik Csapak wrote:
>>> +		    getClass: (v, m, rec) => rec.data.ty === 'dir' ? 'fa fa-info-circle' : 'pmx-hidden',
>>
>> info-circle is not a good icon for some specific stats, i.e., not a
>> general info about the backup snapshot.. A stop-watch could be nice,
>> but there doesn't seem to be any, so possibly "fa-file-text-o" (a
>> sheet of stat lines, so to say), not ideal too but IMO better than
>> the info-circle.
>>
>> ps. maybe injecting some more general info like duration could be
>> nice (didn't check if we even have that available already here
>> though).
>>
>> That said maybe one could even make this an actual info dialog,
>> with the stats only be one part of that, then the info-circle
>> could be OK too and we'd not add a core UI element for a rather
>> niche information that most won't look at often.
> 
> here we basically have only the info we have in the grid already,
> but we could provide it in a nicer way maybe:
> 
> backup time, files (+sizes), last verification info (+link to task log), etc.

Yeah, that's what I basically meant first, show the whole info a
bit nicer, possibly even hide some columns of it by default then
(the list is quite cramped already)

> 
> or did you mean we add a new api endpoint that returns more info about the snapshot
> altogether? (which could also make sense)

I mean, then we'd not have to "shove" in the upload stats into the
generic list snapshots API call, as you wrote yourself, especially
if we never plan to show those inline it might make really more
sense to split that, even if we'd have the manifest already read
and thus in memory.

Without an in-depth analysis, I think I'd prefer that slightly
more, especially as the maintenance cost of that extra endpoint
should be rather negligible (if there's a good API endpoint path
to put it in, as that sometimes seems to be the harder part ^^)

And yes, we could then show all the possible data about a
snapshot, even if some of that is currently already included in
the content tree.

Re: [pbs-devel] [PATCH proxmox-backup 3/3] ui: datastore content: add action to show upload statistics

[Dominik Csapak]

On 11/27/23 11:27, Thomas Lamprecht wrote:
> On 27.11.23 11:04, Dominik Csapak wrote:
>>>> +		    getClass: (v, m, rec) => rec.data.ty === 'dir' ? 'fa fa-info-circle' : 'pmx-hidden',
>>>
>>> info-circle is not a good icon for some specific stats, i.e., not a
>>> general info about the backup snapshot.. A stop-watch could be nice,
>>> but there doesn't seem to be any, so possibly "fa-file-text-o" (a
>>> sheet of stat lines, so to say), not ideal too but IMO better than
>>> the info-circle.
>>>
>>> ps. maybe injecting some more general info like duration could be
>>> nice (didn't check if we even have that available already here
>>> though).
>>>
>>> That said maybe one could even make this an actual info dialog,
>>> with the stats only be one part of that, then the info-circle
>>> could be OK too and we'd not add a core UI element for a rather
>>> niche information that most won't look at often.
>>
>> here we basically have only the info we have in the grid already,
>> but we could provide it in a nicer way maybe:
>>
>> backup time, files (+sizes), last verification info (+link to task log), etc.
> 
> Yeah, that's what I basically meant first, show the whole info a
> bit nicer, possibly even hide some columns of it by default then
> (the list is quite cramped already)
> 
>>
>> or did you mean we add a new api endpoint that returns more info about the snapshot
>> altogether? (which could also make sense)
> 
> I mean, then we'd not have to "shove" in the upload stats into the
> generic list snapshots API call, as you wrote yourself, especially
> if we never plan to show those inline it might make really more
> sense to split that, even if we'd have the manifest already read
> and thus in memory.
> 
> Without an in-depth analysis, I think I'd prefer that slightly
> more, especially as the maintenance cost of that extra endpoint
> should be rather negligible (if there's a good API endpoint path
> to put it in, as that sometimes seems to be the harder part ^^)
> 
> And yes, we could then show all the possible data about a
> snapshot, even if some of that is currently already included in
> the content tree.

looking at the code, there really is not much more info about
the backups than what we already have in the tree
(at least not cheap ones from the manifest etc)

the only info we have that is missing from the snapshotlistitem
is the group comment, the key fingerprint and the upload statistics,
so i'm asking myself if that is really worth a seperate api call...

Re: [pbs-devel] [PATCH proxmox-backup 3/3] ui: datastore content: add action to show upload statistics

[Thomas Lamprecht]

On 27.11.23 11:33, Dominik Csapak wrote:
> On 11/27/23 11:27, Thomas Lamprecht wrote:
>> Without an in-depth analysis, I think I'd prefer that slightly
>> more, especially as the maintenance cost of that extra endpoint
>> should be rather negligible (if there's a good API endpoint path
>> to put it in, as that sometimes seems to be the harder part ^^)
>>
>> And yes, we could then show all the possible data about a
>> snapshot, even if some of that is currently already included in
>> the content tree.
> 
> looking at the code, there really is not much more info about
> the backups than what we already have in the tree
> (at least not cheap ones from the manifest etc)
> 
> the only info we have that is missing from the snapshotlistitem
> is the group comment, the key fingerprint and the upload statistics,
> so i'm asking myself if that is really worth a seperate api call...

Not sure if I'd use the abundance of info in an bloated API call as
"excuse" to not add a new one, but further bloat the existing one.

Remember that we want to do a (streaming) API endpoint that returns
nested objects for the datastore content, where we might want to avoid
parsing each manifest, for that it might be useful

It might also be useful for external API users that just want to get the
info of one snapshot without the huge cost of reading all.

And it might be also useful for having more options for a potential
rework of the datastore content UI, e.g., moving comment editing into
that and some other info or even (lesser used) actions too, that then
either isn't added to the new endpoint, or one can opt-out for the
current one.

Note also that a minimal stats entry , e.g.:
"upload-statistic":{"count":0,"size":0,"compressed-size":0,"duplicates":0}

Total to 75 bytes, so for an actual realistic one 100 bytes seems
reasonable, and while transport compression will help, one still needs
to have all that in (browser) memory, not a huge cost, but again going
into the direction we rather want to reverse from.

Did you thought about the new endpoint with above in mind?  I mean sure,
above includes a few rather far future looking assumptions, but not sure
how we ever get away from the current design if we only ever add on top,
as each specifically checked cost own its own was small (it adds up, on
multiple levels).

Re: [pbs-devel] [PATCH proxmox-backup 3/3] ui: datastore content: add action to show upload statistics

[Thomas Lamprecht]

On 27.11.23 11:15, Dominik Csapak wrote:
> On 11/27/23 11:07, Thomas Lamprecht wrote:
>> On 27.11.23 10:28, Thomas Lamprecht wrote:
>>> we'd not add a core UI element for a rather niche information
>>> that most won't look at often.
>>
>> re-thinking this, maybe we want to put those things into a context
>> menu and expose that menu with a fa-bars or fa-toggle-down icon,
>> in addition with allowing one to trigger it with a standard right
>> click context menu (I wanted to add that for datastore content UI
>> since quite a while already).
> 
> sure, doesn't sound too complicated. i'd put the remaining actions
> also there. would you then still prefer a "general info" panel
> over just showing the upload statistics? (i guess so, but want
> to make sure)

Yes, and I'd also like that this is split (patch-wise) a bit so that
we do not have to use all or nothing now.

I.e., add context menu for existing actions and wire it up to right
click, that on it's own is a nice feature and doesn't needs anything
else mixed in.

Then add the new info thingy and add it only there for now.

Then rework a few of the existing actions out and add a (possibly
slightly filtered) fa-bars trigger-able menu there). IMO while the
other ones are relatively clear, this one has the most potential
for different opinion and UX workflow design and I'd not like to
being forced to rush that if I want the other (above) features now
already.

Re: [pbs-devel] [PATCH proxmox-backup 3/3] ui: datastore content: add action to show upload statistics

[Dominik Csapak]

On 11/27/23 13:02, Thomas Lamprecht wrote:
> On 27.11.23 11:33, Dominik Csapak wrote:
>> On 11/27/23 11:27, Thomas Lamprecht wrote:
>>> Without an in-depth analysis, I think I'd prefer that slightly
>>> more, especially as the maintenance cost of that extra endpoint
>>> should be rather negligible (if there's a good API endpoint path
>>> to put it in, as that sometimes seems to be the harder part ^^)
>>>
>>> And yes, we could then show all the possible data about a
>>> snapshot, even if some of that is currently already included in
>>> the content tree.
>>
>> looking at the code, there really is not much more info about
>> the backups than what we already have in the tree
>> (at least not cheap ones from the manifest etc)
>>
>> the only info we have that is missing from the snapshotlistitem
>> is the group comment, the key fingerprint and the upload statistics,
>> so i'm asking myself if that is really worth a seperate api call...
> 
> Not sure if I'd use the abundance of info in an bloated API call as
> "excuse" to not add a new one, but further bloat the existing one.
> 
> Remember that we want to do a (streaming) API endpoint that returns
> nested objects for the datastore content, where we might want to avoid
> parsing each manifest, for that it might be useful
> 
> It might also be useful for external API users that just want to get the
> info of one snapshot without the huge cost of reading all.
> 
> And it might be also useful for having more options for a potential
> rework of the datastore content UI, e.g., moving comment editing into
> that and some other info or even (lesser used) actions too, that then
> either isn't added to the new endpoint, or one can opt-out for the
> current one.
> 
> Note also that a minimal stats entry , e.g.:
> "upload-statistic":{"count":0,"size":0,"compressed-size":0,"duplicates":0}
> 
> Total to 75 bytes, so for an actual realistic one 100 bytes seems
> reasonable, and while transport compression will help, one still needs
> to have all that in (browser) memory, not a huge cost, but again going
> into the direction we rather want to reverse from.
> 
> Did you thought about the new endpoint with above in mind?  I mean sure,
> above includes a few rather far future looking assumptions, but not sure
> how we ever get away from the current design if we only ever add on top,
> as each specifically checked cost own its own was small (it adds up, on
> multiple levels).

you are absolutely right, bloating the existing one even further is going
into the wrong direction, i'll add a new api endpoint for snapshot
information

Re: [pbs-devel] [RFC PATCH proxmox-backup 1/3] api-types: make UploadStatistic an api type

[Wolfgang Bumiller]

On Mon, Nov 27, 2023 at 11:17:49AM +0100, Dominik Csapak wrote:
> On 11/27/23 11:12, Thomas Lamprecht wrote:
> > On 27.11.23 11:01, Dominik Csapak wrote:
> > > On 11/27/23 10:52, Thomas Lamprecht wrote:
> > > > On 01.08.23 11:29, Dominik Csapak wrote:
> > > > > +#[api()]
> > > > > +#[derive(Copy, Clone, Serialize, Deserialize)]
> > > > 
> > > > misses:
> > > > 
> > > > #[serde(rename_all = "kebab-case")]
> > > > 
> > > > or does that break manifest?
> > > 
> > > it shouldn't since we only save it in the 'unprotected' field that is a 'Value'
> > > but i'll check
> > 
> > I did not mean breakage as in "might break signatures", but as in
> > backward/forward compat to any of our code/tools using that field
> > already (tbh. not sure from top of my head if serde json magically
> > falls back to field casing variants, e.g., check if foo-bar is there
> > if not take foo_bar, I think not, so that's why I asked - should have
> > said so in my first response).
> 
> mhmm.. we only ever write that once (during backup finish) and never read
> it anywhere (until my patch) so it couldn't break any existing code.
> 
> adding the rename now would only affect new backups, but you might be
> right that deserializing older backups might not be working then.
> 
> again, i'll check

in theory we could also add a #[serde(alias = "...")]. The docs say
"deserialize (...) from the given name *or* from its Rust name"[1], but
I'm not sure if "its Rust name" then means the original or the
`rename_all`-transformed version ;-)
Also, we don't currently directly support this in the proxmox-schema -
but we do in perl iirc, so we might as well add that to rust?

[1] https://serde.rs/field-attrs.html

Re: [pbs-devel] [RFC PATCH proxmox-backup 1/3] api-types: make UploadStatistic an api type

[Dominik Csapak]

On 11/27/23 13:44, Wolfgang Bumiller wrote:
> On Mon, Nov 27, 2023 at 11:17:49AM +0100, Dominik Csapak wrote:
>> On 11/27/23 11:12, Thomas Lamprecht wrote:
>>> On 27.11.23 11:01, Dominik Csapak wrote:
>>>> On 11/27/23 10:52, Thomas Lamprecht wrote:
>>>>> On 01.08.23 11:29, Dominik Csapak wrote:
>>>>>> +#[api()]
>>>>>> +#[derive(Copy, Clone, Serialize, Deserialize)]
>>>>>
>>>>> misses:
>>>>>
>>>>> #[serde(rename_all = "kebab-case")]
>>>>>
>>>>> or does that break manifest?
>>>>
>>>> it shouldn't since we only save it in the 'unprotected' field that is a 'Value'
>>>> but i'll check
>>>
>>> I did not mean breakage as in "might break signatures", but as in
>>> backward/forward compat to any of our code/tools using that field
>>> already (tbh. not sure from top of my head if serde json magically
>>> falls back to field casing variants, e.g., check if foo-bar is there
>>> if not take foo_bar, I think not, so that's why I asked - should have
>>> said so in my first response).
>>
>> mhmm.. we only ever write that once (during backup finish) and never read
>> it anywhere (until my patch) so it couldn't break any existing code.
>>
>> adding the rename now would only affect new backups, but you might be
>> right that deserializing older backups might not be working then.
>>
>> again, i'll check
> 
> in theory we could also add a #[serde(alias = "...")]. The docs say
> "deserialize (...) from the given name *or* from its Rust name"[1], but
> I'm not sure if "its Rust name" then means the original or the
> `rename_all`-transformed version ;-)
> Also, we don't currently directly support this in the proxmox-schema -
> but we do in perl iirc, so we might as well add that to rust?
> 
> [1] https://serde.rs/field-attrs.html


btw. i can confirm that if i use serdes rename_all = "kebab-case" we
can't deserialize it anymore (did not find 'compressed-size')
but if i add alias = "compressed_size" it can deserialize from
existing manifests, and serializes as 'compressed-size'