Carry out an extra http request for warmup without considering for th…

[danielealbano]

This PR changes how the HTTPPing calculates the latency doing a connection and dns warmup triggering an extra request at the beginning.

Meanwhile this will force the HTTPPing to carry out a de-facto unwanted request, it's a necessity to ensure that the initial connection will not impact the overall estimation.

This, though, is only effective when an HTTP/2 or an HTTP/1.1 with keep-alive is established, if none of the two work the latencies reported will be all over the place.

An improvement to this PR might be to validate that an HTTP/2 or an HTTP/1.1 with keepalive enabled is in use and otherwise raise an exception.

Tested with this code

package main

import (
	"context"
	"fmt"
	"github.com/showwin/speedtest-go/speedtest"
	"time"
)

func httpPing(s *speedtest.Server) []int64 {
	var latenciesInNs []int64
	var err error

	if latenciesInNs, err = s.HTTPPing(context.Background(), 20, 250*time.Millisecond, nil); err != nil {
		fmt.Printf("error while trying to ping the server: %v\n", err)
		return []int64{}
	}
	return latenciesInNs
}

func tcpPing(s *speedtest.Server) []int64 {
	var latenciesInNs []int64
	var err error

	if latenciesInNs, err = s.TCPPing(context.Background(), 20, 250*time.Millisecond, nil); err != nil {
		fmt.Printf("error while trying to ping the server: %v\n", err)
		return []int64{}
	}

	return latenciesInNs
}

func icmpPing(s *speedtest.Server) []int64 {
	var latenciesInNs []int64
	var err error

	if latenciesInNs, err = s.ICMPPing(context.Background(), 5*time.Second, 20, 250*time.Millisecond, nil); err != nil {
		fmt.Printf("error while trying to ping the server: %v\n", err)
		return []int64{}
	}

	return latenciesInNs
}

func printLatencies(latenciesInNs []int64) {
	meanLatency, _, jitter, minLatency, maxLatency := speedtest.StandardDeviation(latenciesInNs)
	fmt.Printf("    Mean Latency: %f\n", float64(meanLatency)/1000000)
	fmt.Printf("    Jitter: %f\n", float64(jitter)/1000000)
	fmt.Printf("    Min Latency: %f\n", float64(minLatency)/1000000)
	fmt.Printf("    Max Latency: %f\n", float64(maxLatency)/1000000)
}

func main() {
	speedTestClient := speedtest.New()
	serverList, _ := speedTestClient.FetchServers()
	targets, _ := serverList.FindServer([]int{})
	for _, s := range targets {
		fmt.Printf("Server: %s (%s)\n", s.Name, s.ID)
		fmt.Printf("HTTP Ping\n")
		printLatencies(httpPing(s))
		fmt.Printf("TCP Ping\n")
		printLatencies(tcpPing(s))
		fmt.Printf("ICMP Ping\n")
		printLatencies(icmpPing(s))
	}
}

Example output

Server: Nottwil (63799)
HTTP Ping
    Mean Latency: 6.351436
    Jitter: 0.369716
    Min Latency: 5.938069
    Max Latency: 7.518020
TCP Ping
    Mean Latency: 5.678754
    Jitter: 0.193643
    Min Latency: 5.338031
    Max Latency: 6.242632
ICMP Ping
    Mean Latency: 5.904735
    Jitter: 0.321297
    Min Latency: 5.498763
    Max Latency: 7.067122

Meanwhile the max latency is greater for HTTP Ping compared ot the a;ternatives, the mean latency is close enough to the latency reported by ICMP Ping

[danielealbano]

Small note: probably HTTP/2 here is not supported altogether as it requires TLS but HTTP/1.1 with keep alive should. Of course the extra latency on the HTTP Ping compared to the alternative is to be expected even just because the server side has to be some extra work compared to the TCPPing or ICMPPing.

[r3inbowari]

My concern is that assuming we send 10 HTTP ping requests and N(N <= 10) ports are used in those 10 requests.
Then the result would be N with a latency of 2RTT, and 10-N with a RTT. I can reproduce this on wireshark.

[danielealbano]

The requests are sent sequentially, not in parallel, and forcing the keep alive in the http client would be enough to guarantee that any sequential request done within the keep alive time frame will be handled correctly.

Howerver, the current solution is just incorrect as it assumes that there are only 2 sets of packets sent but it's never the case.

Even doing a divided by two on the first request still keeps it on the wrong side, it should be more of a /3 or even a /4 but would need to be validated.

[r3inbowari]

You're right, But I think the best way is to disable keep alive for http ping.

[danielealbano]

@r3inbowari I am not entirely sure it's a good idea to disable it, it's the only thing that guarantees that there will be only one packet forth and one packet back during the measurement.

With the tcp/ip ping you have the connection established in advance so you don't need to deal with the extra traffic generated by the initial handshake and the dns resolution, which is a very variable aspect that is triggered only once and goes through a totally separate route (therefore you can't simply do a divided by something).

[r3inbowari]

Yes :) Thank you!