Merge pull request #3 from wbollock/fix/concurrency_lock

fix: scope metric variables to collector function
linode-obs · Dec 14, 2023 · b0c0689 · b0c0689
2 parents add5683 + ea7e384
commit b0c0689
Show file tree

Hide file tree

Showing 3 changed files with 153 additions and 117 deletions.
diff --git a/internal/collector/icmp_collector.go b/internal/collector/icmp_collector.go
@@ -1,7 +1,6 @@
 package collector
 
 import (
-	"math"
 	"net/http"
 	"strconv"
 	"strings"
@@ -11,54 +10,7 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 	log "github.com/sirupsen/logrus"
-)
-
-const (
-	namespace       = "ping_"
-	defaultTimeout  = time.Second * 10
-	defaultInterval = time.Second
-	defaultCount    = 5
-	defaultSize     = 56
-	defaultTTL      = 64
-	defaultProtocol = "ip4"  // or ip6
-	defaultPacket   = "icmp" // or udp
-	maxPacketSize   = 65507
-	minPacketSize   = 24
-)
-
-var (
-	pingSuccessGauge = prometheus.NewGauge(prometheus.GaugeOpts{
-		Name: namespace + "success",
-		Help: "Returns whether the ping succeeded",
-	})
-	pingTimeoutGauge = prometheus.NewGauge(prometheus.GaugeOpts{
-		Name: namespace + "timeout",
-		Help: "Returns whether the ping failed by timeout",
-	})
-	probeDurationGauge = prometheus.NewGauge(prometheus.GaugeOpts{
-		Name: namespace + "duration_seconds",
-		Help: "Returns how long the probe took to complete in seconds",
-	})
-	minGauge = prometheus.NewGauge(prometheus.GaugeOpts{
-		Name: namespace + "rtt_min_seconds",
-		Help: "Best round trip time",
-	})
-	maxGauge = prometheus.NewGauge(prometheus.GaugeOpts{
-		Name: namespace + "rtt_max_seconds",
-		Help: "Worst round trip time",
-	})
-	avgGauge = prometheus.NewGauge(prometheus.GaugeOpts{
-		Name: namespace + "rtt_avg_seconds",
-		Help: "Mean round trip time",
-	})
-	stddevGauge = prometheus.NewGauge(prometheus.GaugeOpts{
-		Name: namespace + "rtt_std_deviation",
-		Help: "Standard deviation",
-	})
-	lossGauge = prometheus.NewGauge(prometheus.GaugeOpts{
-		Name: namespace + "loss_ratio",
-		Help: "Packet loss from 0 to 100",
-	})
+	"github.com/wbollock/ping_exporter/internal/metrics"
 )
 
 type pingParams struct {
@@ -75,6 +27,18 @@ type pingParams struct {
 func parseParams(r *http.Request) pingParams {
 	params := r.URL.Query()
 
+	const (
+		defaultTimeout  = time.Second * 10
+		defaultInterval = time.Second
+		defaultCount    = 5
+		defaultSize     = 56
+		defaultTTL      = 64
+		defaultProtocol = "ip4"  // or ip6
+		defaultPacket   = "icmp" // or udp
+		maxPacketSize   = 65507
+		minPacketSize   = 24
+	)
+
 	p := pingParams{
 		target:   params.Get("target"),
 		timeout:  defaultTimeout,
@@ -146,73 +110,125 @@ func serveMetricsWithError(w http.ResponseWriter, r *http.Request, registry *pro
 	}
 }
 
-func PingHandler(w http.ResponseWriter, r *http.Request) {
-	p := parseParams(r)
-	start := time.Now()
-
-	registry := prometheus.NewRegistry()
-	registry.MustRegister(pingSuccessGauge, pingTimeoutGauge, probeDurationGauge, minGauge, maxGauge, avgGauge, stddevGauge, lossGauge)
+func PingHandler() http.HandlerFunc {
+	return func(w http.ResponseWriter, r *http.Request) {
+
+		const (
+			namespace = "ping"
+		)
+
+		var (
+			pingSuccessGauge = prometheus.NewGauge(prometheus.GaugeOpts{
+				Namespace: namespace,
+				Name:      "success",
+				Help:      "Returns whether the ping succeeded",
+			})
+			pingTimeoutGauge = prometheus.NewGauge(prometheus.GaugeOpts{
+				Namespace: namespace,
+				Name:      "timeout",
+				Help:      "Returns whether the ping failed by timeout",
+			})
+			probeDurationGauge = prometheus.NewGauge(prometheus.GaugeOpts{
+				Namespace: namespace,
+				Name:      "duration_seconds",
+				Help:      "Returns how long the probe took to complete in seconds",
+			})
+			minGauge = prometheus.NewGauge(prometheus.GaugeOpts{
+				Namespace: namespace,
+				Name:      "rtt_min_seconds",
+				Help:      "Best round trip time",
+			})
+			maxGauge = prometheus.NewGauge(prometheus.GaugeOpts{
+				Namespace: namespace,
+				Name:      "rtt_max_seconds",
+				Help:      "Worst round trip time",
+			})
+			avgGauge = prometheus.NewGauge(prometheus.GaugeOpts{
+				Namespace: namespace,
+				Name:      "rtt_avg_seconds",
+				Help:      "Mean round trip time",
+			})
+			stddevGauge = prometheus.NewGauge(prometheus.GaugeOpts{
+				Namespace: namespace,
+				Name:      "rtt_std_deviation",
+				Help:      "Standard deviation",
+			})
+			lossGauge = prometheus.NewGauge(prometheus.GaugeOpts{
+				Namespace: namespace,
+				Name:      "loss_ratio",
+				Help:      "Packet loss from 0 to 100",
+			})
+		)
+
+		metrics := metrics.PingMetrics{
+			PingSuccessGauge:   pingSuccessGauge,
+			PingTimeoutGauge:   pingTimeoutGauge,
+			ProbeDurationGauge: probeDurationGauge,
+			MinGauge:           minGauge,
+			MaxGauge:           maxGauge,
+			AvgGauge:           avgGauge,
+			StddevGauge:        stddevGauge,
+			LossGauge:          lossGauge,
+		}
+		registry := prometheus.NewRegistry()
 
-	// assume failure
-	pingSuccessGauge.Set(0)
-	pingTimeoutGauge.Set(1)
+		registry.MustRegister(metrics.PingSuccessGauge, metrics.PingTimeoutGauge, metrics.ProbeDurationGauge, metrics.MinGauge, metrics.MaxGauge, metrics.AvgGauge, metrics.StddevGauge, metrics.LossGauge)
 
-	log.Debugf("Request received with parameters: target=%v, count=%v, size=%v, interval=%v, timeout=%v, ttl=%v, packet=%v",
-		p.target, p.count, p.size, p.interval, p.timeout, p.ttl, p.packet)
+		p := parseParams(r)
+		start := time.Now()
 
-	pinger := probing.New(p.target)
+		log.Debugf("Request received with parameters: target=%v, count=%v, size=%v, interval=%v, timeout=%v, ttl=%v, packet=%v",
+			p.target, p.count, p.size, p.interval, p.timeout, p.ttl, p.packet)
 
-	pinger.Count = p.count
-	pinger.Size = p.size
-	pinger.Interval = p.interval
-	pinger.Timeout = p.timeout
-	pinger.TTL = p.ttl
+		pinger := probing.New(p.target)
 
-	if p.packet == "icmp" {
-		pinger.SetPrivileged(true)
-	} else {
-		pinger.SetPrivileged(false)
-	}
+		pinger.Count = p.count
+		pinger.Size = p.size
+		pinger.Interval = p.interval
+		pinger.Timeout = p.timeout
+		pinger.TTL = p.ttl
 
-	if p.protocol == "v6" || p.protocol == "6" || p.protocol == "ip6" {
-		pinger.SetNetwork("ip6")
-	} else {
-		pinger.SetNetwork("ip4")
-	}
+		if p.packet == "icmp" {
+			pinger.SetPrivileged(true)
+		} else {
+			pinger.SetPrivileged(false)
+		}
 
-	pinger.OnRecv = func(pkt *probing.Packet) {
-		log.Debugf("%d bytes from %s: icmp_seq=%d time=%v ttl=%v\n",
-			pkt.Nbytes, pkt.IPAddr, pkt.Seq, pkt.Rtt, pkt.TTL)
-	}
+		if p.protocol == "v6" || p.protocol == "6" || p.protocol == "ip6" {
+			pinger.SetNetwork("ip6")
+		} else {
+			pinger.SetNetwork("ip4")
+		}
 
-	pinger.OnDuplicateRecv = func(pkt *probing.Packet) {
-		log.Debugf("%d bytes from %s: icmp_seq=%d time=%v ttl=%v (DUP!)\n",
-			pkt.Nbytes, pkt.IPAddr, pkt.Seq, pkt.Rtt, pkt.TTL)
-	}
+		pinger.OnFinish = func(stats *probing.Statistics) {
+			log.Debugf("OnFinish: target=%v, PacketsSent=%d, PacketsRecv=%d, PacketLoss=%f%%, MinRtt=%v, AvgRtt=%v, MaxRtt=%v, StdDevRtt=%v, Duration=%v",
+				stats.IPAddr, pinger.PacketsSent, pinger.PacketsRecv, stats.PacketLoss, stats.MinRtt, stats.AvgRtt, stats.MaxRtt, stats.StdDevRtt, time.Since(start))
+
+			if pinger.PacketsRecv > 0 && pinger.Timeout > time.Since(start) {
+				log.Debugf("Ping successful: target=%v", stats.IPAddr)
+				metrics.PingSuccessGauge.Set(1)
+				metrics.PingTimeoutGauge.Set(0)
+			} else if pinger.Timeout < time.Since(start) {
+				log.Infof("Ping timeout: target=%v, timeout=%v, duration=%v", stats.IPAddr, pinger.Timeout, time.Since(start))
+				metrics.PingTimeoutGauge.Set(1)
+				metrics.PingSuccessGauge.Set(0)
+			} else if pinger.PacketsRecv == 0 {
+				log.Infof("Ping failed, no packets received: target=%v, packetsRecv=%v, packetsSent=%v", stats.IPAddr, pinger.PacketsRecv, pinger.PacketsSent)
+				metrics.PingSuccessGauge.Set(0)
+				metrics.PingTimeoutGauge.Set(0)
+			}
 
-	pinger.OnFinish = func(stats *probing.Statistics) {
-		log.Debugf("OnFinish: PacketsSent=%d, PacketsRecv=%d, PacketLoss=%f%%, MinRtt=%v, AvgRtt=%v, MaxRtt=%v, StdDevRtt=%v, Duration=%v",
-			pinger.PacketsSent, pinger.PacketsRecv, stats.PacketLoss, stats.MinRtt, stats.AvgRtt, stats.MaxRtt, stats.StdDevRtt, time.Since(start))
-
-		const tolerance = 0.001 // tolerance for easier floating point comparisons
-		// success declared if we sent as many packets as intended, we got back all packets sent, and packet loss is not equal to 100%
-		if pinger.Count == pinger.PacketsRecv && math.Abs(stats.PacketLoss-100) > tolerance {
-			// no error will be raised if we reach a timeout
-			// https://github.com/prometheus-community/pro-bing/issues/70
-			pingSuccessGauge.Set(1)
-			pingTimeoutGauge.Set(0)
+			metrics.MinGauge.Set(stats.MinRtt.Seconds())
+			metrics.AvgGauge.Set(stats.AvgRtt.Seconds())
+			metrics.MaxGauge.Set(stats.MaxRtt.Seconds())
+			metrics.StddevGauge.Set(float64(stats.StdDevRtt))
+			metrics.LossGauge.Set(stats.PacketLoss)
+			metrics.ProbeDurationGauge.Set(time.Since(start).Seconds())
 		}
-		minGauge.Set(stats.MinRtt.Seconds())
-		avgGauge.Set(stats.AvgRtt.Seconds())
-		maxGauge.Set(stats.MaxRtt.Seconds())
-		stddevGauge.Set(float64(stats.StdDevRtt))
-		lossGauge.Set(stats.PacketLoss)
-		probeDurationGauge.Set(time.Since(start).Seconds())
-	}
 
-	if err := pinger.Run(); err != nil {
-		log.Error("Failed to ping target host:", err)
+		if err := pinger.Run(); err != nil {
+			log.Error("Failed to ping target host:", err)
+		}
 		serveMetricsWithError(w, r, registry)
 	}
-	serveMetricsWithError(w, r, registry)
 }
diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go
@@ -0,0 +1,16 @@
+package metrics
+
+import (
+	"github.com/prometheus/client_golang/prometheus"
+)
+
+type PingMetrics struct {
+	PingSuccessGauge   prometheus.Gauge
+	PingTimeoutGauge   prometheus.Gauge
+	ProbeDurationGauge prometheus.Gauge
+	MinGauge           prometheus.Gauge
+	MaxGauge           prometheus.Gauge
+	AvgGauge           prometheus.Gauge
+	StddevGauge        prometheus.Gauge
+	LossGauge          prometheus.Gauge
+}
diff --git a/internal/server/server.go b/internal/server/server.go
@@ -10,22 +10,26 @@ import (
 	"github.com/wbollock/ping_exporter/internal/collector"
 )
 
-const (
-	defaultHTML = `<html>
-			<head><title>Ping Exporter</title></head>
-			<body>
-			<h1>Ping Exporter</h1>
-			<p><a href='%s'>Metrics</a></p>
-			</body>
-			</html>`
-	defaultMetricsPath = "/metrics"
-)
-
 func SetupServer() http.Handler {
+
+	const (
+		defaultHTML = `<html>
+				<head><title>Ping Exporter</title></head>
+				<body>
+				<h1>Ping Exporter</h1>
+				<p><a href='%s'>Metrics</a></p>
+				</body>
+				</html>`
+		defaultMetricsPath = "/metrics"
+	)
+
 	mux := http.NewServeMux()
 
 	mux.Handle(defaultMetricsPath, promhttp.Handler())
-	mux.HandleFunc("/probe", collector.PingHandler)
+
+	pingHandler := collector.PingHandler()
+
+	mux.HandleFunc("/probe", pingHandler)
 
 	// for non-standard web servers, need to register handlers
 	mux.HandleFunc("/debug/pprof/", http.HandlerFunc(pprof.Index))