Include service name and version in User-Agent

[axw]

Update User-Agent to match the agent spec, including service name and service version if it is specified. User-Agent no longer includes the Go runtime version, but this is still available in service.runtime.version.

The transport.NewHTTPTransportOptions function is removed, and transport.NewHTTPTransport is updated to take HTTPTransportOptions instead now that we can break the API. Use env vars as defaults only if values are not specified in the options struct, aligning with behaviour of apm.NewTracerOptions.

Update to the latest shared agents Gherkin spec.

Closes #1136

[apmmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2022-01-19T05:36:24.720+0000

• Duration: 31 min 7 sec

• Commit: bc577fc

Test stats 🧪

Test	Results
Failed	0
Passed	7214
Skipped	174
Total	7388

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

• run benchmark tests : Run the benchmark test.

• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[stuartnelson3]

looks good! small question, but that's all.

[stuartnelson3]

taking that back 😅 this is good to go once the tls config options get rolled into a *tls.Config

changes already implemented

[axw]

@stuartnelson3 merging as I think you concluded that all necessary changes were already made. Let me know if I misinterpreted.

[marclop]

Tested with the latest main (640a916) and I believe we forgot to update the code for http.NewTransport(), since passing the ServiceName and ServiceVersion has no effect on custom transports:

package main

import (
	"fmt"
	"io"
	"net/http"
	"net/http/httptest"
	"net/url"

	"go.elastic.co/apm/v2"
	"go.elastic.co/apm/v2/transport"
)

func main() {
	dumpHandler := func(m string) http.HandlerFunc {
		return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
			defer r.Body.Close()
			io.Copy(io.Discard, r.Body)
			fmt.Println(m, "\t", "URL:", r.URL.Path, "User-Agent:", r.Header.Get("User-Agent"))
			rw.WriteHeader(202)
		})
	}

	s := http.Server{
		Addr:    "localhost:8200",
		Handler: dumpHandler("default"),
	}
	defer s.Close()
	go s.ListenAndServe()

	srv := httptest.NewServer(dumpHandler("custom "))
	defer srv.Close()
	u, err := url.Parse(srv.URL)
	panicOnErr(err)

	t, err := transport.NewHTTPTransport(transport.HTTPTransportOptions{
		ServerURLs: []*url.URL{u},
	})
	panicOnErr(err)

	tracer, err := apm.NewTracerOptions(apm.TracerOptions{
		ServiceName:    "marc-test",
		ServiceVersion: "5.1.3",
		Transport:      t,
	})

	genTraces(tracer)
	genTraces(apm.DefaultTracer())

	tracer.Flush(nil)
	apm.DefaultTracer().Flush(nil)
	tracer.Close()
	apm.DefaultTracer().Close()
}

func panicOnErr(err error) {
	if err != nil {
		panic(err)
	}
}

func genTraces(tracer *apm.Tracer) {
	tx := tracer.StartTransaction("tx", "type")
	tx.StartSpan("span", "type", nil).End()
	tx.End()
}

yields:

$ export ELASTIC_APM_SERVICE_NAME=marc-test
$ export ELASTIC_APM_SERVICE_VERSION="5.3.1"
$ go run main.go
custom  	 URL: /config/v1/agents User-Agent: apm-agent-go/2.0.0
default 	 URL: /config/v1/agents User-Agent: apm-agent-go/2.0.0 (marc-test 5.3.1)
custom  	 URL: /intake/v2/events User-Agent: apm-agent-go/2.0.0
default 	 URL: /intake/v2/events User-Agent: apm-agent-go/2.0.0 (marc-test 5.3.1)

[axw]

UserAgent needs to be set in HTTPTransportOptions:

apm-agent-go/transport/http.go

Lines 129 to 133 in 640a916

	// UserAgent holds the value to use for the User-Agent header.
	//
	// If unspecified, UserAgent will be set to the value returned by
	// DefaultUserAgent().
	UserAgent string

Maybe not the most user-friendly. Any thoughts on how to improve that?

[marclop]

ah I didn't read the UserAgent comment, it was unexpected to me since thought it should've also extended the UserAgent when not explicitly set and the ServiceName and ServiceVersion was specified in HTTPTransportOptions.

tracer, err := apm.NewTracerOptions(apm.TracerOptions{
	Transport: t,
}) // Yields UserAgent = `apm-agent-go/2.0.0`

tracer, err := apm.NewTracerOptions(apm.TracerOptions{
	ServiceName:    "marc-test",
	ServiceVersion: "5.1.3",
	Transport:      t,
}) // Yields UserAgent = `apm-agent-go/2.0.0 (marc-test 5.3.1)`

tracer, err := apm.NewTracerOptions(apm.TracerOptions{
	ServiceName:    "marc-test",
	ServiceVersion: "5.1.3",
	Transport:      t,
	UserAgent:      "custom-user-agent"
}) // Yields UserAgent = `custom-user-agent`

It isn't a regression then since the mechanics are clearly defined in the godocs.

[axw]

Maybe we can improve this later, e.g. by adding a SetUserAgent method to the Transport interface. Then NewTracerOptions could update it. I prefer it being explicitly specified, but I can see how it's a bit surprising.