[pve-devel] [PATCH container v3] fix #3815: influxdb vmname should be string

[pve-devel] [PATCH container v3] fix #3815: influxdb vmname should be string

[Markus Frank]

InfluxDB interprets the vmname 66601 as a number and the vmname vm42 as a String.
This leads to problematic metrics, that will be dropped by influxdb.
Whichever comes first decides how the "schema" is defined.

To change that I added a $to_quote hashmap to define which value
shouldn't get interpreted as number.
In this case the value of name.

Change: Conversion happends in prepare_value.

nodename and host are tags in InfluxDB so the only value they are able
to contain are strings:
https://docs.influxdata.com/influxdb/v2.1/reference/syntax/line-protocol/

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
 PVE/Status/InfluxDB.pm | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/PVE/Status/InfluxDB.pm b/PVE/Status/InfluxDB.pm
index def7e2fd..63a865df 100644
--- a/PVE/Status/InfluxDB.pm
+++ b/PVE/Status/InfluxDB.pm
@@ -276,6 +276,8 @@ sub test_connection {
 sub build_influxdb_payload {
     my ($class, $txn, $data, $ctime, $tags, $excluded, $measurement, $instance) = @_;
 
+    # 'abc' and '123' are both valid hostnames, that confuses influx's type detection
+    my $to_quote = { name => 1 };
     my @values = ();
 
     foreach my $key (sort keys %$data) {
@@ -286,7 +288,7 @@ sub build_influxdb_payload {
 	if (!ref($value) && $value ne '') {
 	    # value is scalar
 
-	    if (defined(my $v = prepare_value($value))) {
+	    if (defined(my $v = prepare_value($value, $to_quote->{$key}))) {
 		push @values, "$key=$v";
 	    }
 	} elsif (ref($value) eq 'HASH') {
@@ -331,9 +333,10 @@ sub get_recursive_values {
 }
 
 sub prepare_value {
-    my ($value) = @_;
+    my ($value, $quote) = @_;
 
-    if (looks_like_number($value)) {
+    # don't treat value like a number if quote is 1
+    if (looks_like_number($value) && !$quote) {
 	if (isnan($value) || isinf($value)) {
 	    # we cannot send influxdb NaN or Inf
 	    return undef;
-- 
2.30.2

Re: [pve-devel] [PATCH manager v3] fix #3815: influxdb vmname should be string

[Markus Frank]

manager not container

On 1/28/22 11:03, Markus Frank wrote:
> InfluxDB interprets the vmname 66601 as a number and the vmname vm42 as a String.
> This leads to problematic metrics, that will be dropped by influxdb.
> Whichever comes first decides how the "schema" is defined.
>
> To change that I added a $to_quote hashmap to define which value
> shouldn't get interpreted as number.
> In this case the value of name.
>
> Change: Conversion happends in prepare_value.
>
> nodename and host are tags in InfluxDB so the only value they are able
> to contain are strings:
> https://docs.influxdata.com/influxdb/v2.1/reference/syntax/line-protocol/
>
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>   PVE/Status/InfluxDB.pm | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/PVE/Status/InfluxDB.pm b/PVE/Status/InfluxDB.pm
> index def7e2fd..63a865df 100644
> --- a/PVE/Status/InfluxDB.pm
> +++ b/PVE/Status/InfluxDB.pm
> @@ -276,6 +276,8 @@ sub test_connection {
>   sub build_influxdb_payload {
>       my ($class, $txn, $data, $ctime, $tags, $excluded, $measurement, $instance) = @_;
>   
> +    # 'abc' and '123' are both valid hostnames, that confuses influx's type detection
> +    my $to_quote = { name => 1 };
>       my @values = ();
>   
>       foreach my $key (sort keys %$data) {
> @@ -286,7 +288,7 @@ sub build_influxdb_payload {
>   	if (!ref($value) && $value ne '') {
>   	    # value is scalar
>   
> -	    if (defined(my $v = prepare_value($value))) {
> +	    if (defined(my $v = prepare_value($value, $to_quote->{$key}))) {
>   		push @values, "$key=$v";
>   	    }
>   	} elsif (ref($value) eq 'HASH') {
> @@ -331,9 +333,10 @@ sub get_recursive_values {
>   }
>   
>   sub prepare_value {
> -    my ($value) = @_;
> +    my ($value, $quote) = @_;
>   
> -    if (looks_like_number($value)) {
> +    # don't treat value like a number if quote is 1
> +    if (looks_like_number($value) && !$quote) {
>   	if (isnan($value) || isinf($value)) {
>   	    # we cannot send influxdb NaN or Inf
>   	    return undef;

[pve-devel] applied: [PATCH container v3] fix #3815: influxdb vmname should be string

[Thomas Lamprecht]

On 28.01.22 11:03, Markus Frank wrote:
> InfluxDB interprets the vmname 66601 as a number and the vmname vm42 as a String.

note, above line was still to long, fixed that on applying

> This leads to problematic metrics, that will be dropped by influxdb.
> Whichever comes first decides how the "schema" is defined.
> 
> To change that I added a $to_quote hashmap to define which value
> shouldn't get interpreted as number.
> In this case the value of name.
> 
> Change: Conversion happends in prepare_value.

FYI, changelogs for patch revisions should rather go --->

> 
> nodename and host are tags in InfluxDB so the only value they are able
> to contain are strings:
> https://docs.influxdata.com/influxdb/v2.1/reference/syntax/line-protocol/
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---


----> here, as they are only relevant for review and do not matter for
the actual committed patch, albeit the reason for the final approach
may matter (but that can be seen as independent from the review history)

>  PVE/Status/InfluxDB.pm | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
>

applied, thanks! 

> @@ -331,9 +333,10 @@ sub get_recursive_values {
>  }
>  
>  sub prepare_value {
> -    my ($value) = @_;
> +    my ($value, $quote) = @_;

IMO $quote is not ideal here as it implies that we only quote if that param
is true, so I renamed it to $force_quote in a followup commit

>  
> -    if (looks_like_number($value)) {
> +    # don't treat value like a number if quote is 1
> +    if (looks_like_number($value) && !$quote) {

Ordering the !$force_quote early allows perl to skip the more expensive 
looks_like_number check if quoting is forced anyway (fixed in the same followup
I made for renaming the parameter name).

Anyhow, just rather small nits and mentioning it here only fyi/for the record.
Thx!