Remove invalid UTF-8 data.

[lyda]

Without this it panics with bad UTF-8 data which means we lose those hits.

[aldas]

Please provide some executable example for this problem

[aldas]

and are you sure this is the right place for this fix? any middleware or handler could do something with that "bad UTF8" url value.

and why statuscode needs to be replaced?
could method be bad utf8?

[lyda]

// Package main sends an invalid http request
package main

import (
  "fmt"
  "net"
)

func main() {
  conn, _ := net.Dial("tcp", "127.0.0.1:8080")
  data := []byte("GET /\xc0 HTTP/1.1\r\nHost: example.com\r\n\r\n")
  conn.Write(data)
  buf := make([]byte, 5000)
  conn.Read(buf)
  fmt.Println(string(buf[:]))
}

This will result in a panic when this middleware is used. So far I've only seen this for the path so I could change it to only include that. I've seen such requests in random probes of sites I look after. This is the only middleware that crashes that I've seen (but I don't use a lot of them).

[aldas]

Seems that https://github.com/prometheus/client_golang/blob/853c5de11e8a0ee0742689859f1642996517b479/prometheus/histogram.go#L1197 this method panics because strings are validated for being proper utf8 here https://github.com/prometheus/client_golang/blob/853c5de11e8a0ee0742689859f1642996517b479/prometheus/labels.go#L178

so I propose we change these lines

echo-contrib/echoprometheus/prometheus.go

Lines 279 to 282 in 415ca6f

	requestDuration.WithLabelValues(values...).Observe(elapsed)
	requestCount.WithLabelValues(values...).Inc()
	requestSize.WithLabelValues(values...).Observe(float64(reqSz))
	responseSize.WithLabelValues(values...).Observe(float64(c.Response().Size))

to

if obs, tmpErr := requestDuration.GetMetricWithLabelValues(values...); tmpErr == nil {
	obs.Observe(elapsed)
} else {
	return fmt.Errorf("requestDuration observation failed, err: %w", tmpErr)
}

if obs, tmpErr := requestCount.GetMetricWithLabelValues(values...); tmpErr == nil {
	obs.Inc()
} else {
	return fmt.Errorf("requestCount observation failed, err: %w", tmpErr)
}

if obs, tmpErr := requestSize.GetMetricWithLabelValues(values...); tmpErr == nil {
	obs.Observe(float64(reqSz))
} else {
	return fmt.Errorf("requestSize observation failed, err: %w", tmpErr)
}

if obs, tmpErr := responseSize.GetMetricWithLabelValues(values...); tmpErr == nil {
	obs.Observe(float64(c.Response().Size))
} else {
	return fmt.Errorf("responseSize observation failed, err: %w", tmpErr)
}

[lyda]

OK, logging the error would be good. You only need to do the first one though. After that you know the values are ok.

Edit: See the latest commit for that.

[lyda]

Ping?

[aldas]

Please add at least one test that hows this problem is fixed.

[aldas]

I do not think this is necessary. It should be up to valueFunc creator to handle this.

[lyda]

In the end I made a number of changes to this to make it "safe by default" in a kubernetes context. It handles broken utf-8 in all fields and it avoids remote-driven cardinality explosions by default. I also simplified the code a bit. I have a bunch of other things to do so if you'd like some or all of those changes feel free to take them. It meets my needs now so I'll just use that. Thanks!

https://gitlab.com/lyda/echoprometheus