Modifying how we handle OTEL traces/metrics endpoints

docs/sources/configure/options.md

@@ -237,6 +237,11 @@ The `OTEL_EXPORTER_OTLP_ENDPOINT` environment variable sets a common endpoint fo
 or the `endpoint` YAML, property will set the endpoint only for the metrics exporter node,
 such that the traces' exporter won't be activated unless explicitly specified.
 
+According to the OpenTelemetry standard, if you set the endpoint via the `OTEL_EXPORTER_OTLP_ENDPOINT` environment variable,
+the OpenTelemetry exporter will automatically add the `/v1/metrics` path to the URL. If you want to avoid this
+addition, you can use either the `OTEL_EXPORTER_OTLP_METRICS_ENDPOINT` environment variable or the `environment` YAML
+property to use exactly the provided URL without any addition.
+
 | YAML       | Env var                                                                    | Type   | Default        |
 | ---------- | -------------------------------------------------------------------------- | ------ | -------------- |
 | `protocol` | `OTEL_EXPORTER_OTLP_PROTOCOL` or<br/>`OTEL_EXPORTER_OTLP_METRICS_PROTOCOL` | string | `http/protobuf |
@@ -353,6 +358,11 @@ The `OTEL_EXPORTER_OTLP_ENDPOINT` environment variable sets a common endpoint fo
 or the `endpoint` YAML property, will set the endpoint only for the traces' exporter node,
 so the metrics exporter won't be activated unless explicitly specified.
 
+According to the OpenTelemetry standard, if you set the endpoint via the `OTEL_EXPORTER_OTLP_ENDPOINT` environment variable,
+the OpenTelemetry exporter will automatically add the `/v1/traces` path to the URL. If you want to avoid this
+addition, you can use either the `OTEL_EXPORTER_OTLP_TRACES_ENDPOINT` environment variable or the `environment` YAML
+property to use exactly the provided URL without any addition.
+
 | YAML       | Env var                                                                   | Type   | Default        |
 | ---------- | ------------------------------------------------------------------------- | ------ | -------------- |
 | `protocol` | `OTEL_EXPORTER_OTLP_PROTOCOL` or<br/>`OTEL_EXPORTER_OTLP_TRACES_PROTOCOL` | string | `http/protobuf |

pkg/internal/export/otel/common.go

@@ -1,12 +1,18 @@
 package otel
 
 import (
+	"crypto/tls"
 	"fmt"
 
 	"github.com/hashicorp/golang-lru/v2/simplelru"
 	"go.opentelemetry.io/otel/attribute"
+	"go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc"
+	"go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp"
+	"go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc"
+	"go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp"
 	"go.opentelemetry.io/otel/sdk/resource"
 	semconv "go.opentelemetry.io/otel/semconv/v1.17.0"
+	"google.golang.org/grpc/credentials"
 )
 
 // Protocol values for the OTEL_EXPORTER_OTLP_PROTOCOL, OTEL_EXPORTER_OTLP_TRACES_PROTOCOL and
@@ -94,3 +100,69 @@ func (rp *ReporterPool[T]) For(svcName string) (T, error) {
 	rp.pool.Add(svcName, m)
 	return m, nil
 }
+
+// Intermediate representation of option functions suitable for testing
+type otlpOptions struct {
+	Endpoint      string
+	Insecure      bool
+	URLPath       string
+	SkipTLSVerify bool
+}
+
+func (o *otlpOptions) AsMetricHTTP() []otlpmetrichttp.Option {
+	opts := []otlpmetrichttp.Option{
+		otlpmetrichttp.WithEndpoint(o.Endpoint),
+	}
+	if o.Insecure {
+		opts = append(opts, otlpmetrichttp.WithInsecure())
+	}
+	if o.URLPath != "" {
+		opts = append(opts, otlpmetrichttp.WithURLPath(o.URLPath))
+	}
+	if o.SkipTLSVerify {
+		opts = append(opts, otlpmetrichttp.WithTLSClientConfig(&tls.Config{InsecureSkipVerify: true}))
+	}
+	return opts
+}
+
+func (o *otlpOptions) AsMetricGRPC() []otlpmetricgrpc.Option {
+	opts := []otlpmetricgrpc.Option{
+		otlpmetricgrpc.WithEndpoint(o.Endpoint),
+	}
+	if o.Insecure {
+		opts = append(opts, otlpmetricgrpc.WithInsecure())
+	}
+	if o.SkipTLSVerify {
+		opts = append(opts, otlpmetricgrpc.WithTLSCredentials(credentials.NewTLS(&tls.Config{InsecureSkipVerify: true})))
+	}
+	return opts
+}
+
+func (o *otlpOptions) AsTraceHTTP() []otlptracehttp.Option {
+	opts := []otlptracehttp.Option{
+		otlptracehttp.WithEndpoint(o.Endpoint),
+	}
+	if o.Insecure {
+		opts = append(opts, otlptracehttp.WithInsecure())
+	}
+	if o.URLPath != "" {
+		opts = append(opts, otlptracehttp.WithURLPath(o.URLPath))
+	}
+	if o.SkipTLSVerify {
+		opts = append(opts, otlptracehttp.WithTLSClientConfig(&tls.Config{InsecureSkipVerify: true}))
+	}
+	return opts
+}
+
+func (o *otlpOptions) AsTraceGRPC() []otlptracegrpc.Option {
+	opts := []otlptracegrpc.Option{
+		otlptracegrpc.WithEndpoint(o.Endpoint),
+	}
+	if o.Insecure {
+		opts = append(opts, otlptracegrpc.WithInsecure())
+	}
+	if o.SkipTLSVerify {
+		opts = append(opts, otlptracegrpc.WithTLSCredentials(credentials.NewTLS(&tls.Config{InsecureSkipVerify: true})))
+	}
+	return opts
+}

pkg/internal/export/otel/common_test.go

@@ -0,0 +1,86 @@
+package otel
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestOtlpOptions_AsMetricHTTP(t *testing.T) {
+	type testCase struct {
+		in  otlpOptions
+		len int
+	}
+	testCases := []testCase{
+		{in: otlpOptions{Endpoint: "foo"}, len: 1},
+		{in: otlpOptions{Endpoint: "foo", Insecure: true}, len: 2},
+		{in: otlpOptions{Endpoint: "foo", URLPath: "/foo"}, len: 2},
+		{in: otlpOptions{Endpoint: "foo", SkipTLSVerify: true}, len: 2},
+		{in: otlpOptions{Endpoint: "foo", Insecure: true, SkipTLSVerify: true}, len: 3},
+		{in: otlpOptions{Endpoint: "foo", URLPath: "/foo", SkipTLSVerify: true}, len: 3},
+		{in: otlpOptions{Endpoint: "foo", URLPath: "/foo", Insecure: true, SkipTLSVerify: true}, len: 4},
+	}
+	for _, tc := range testCases {
+		t.Run(fmt.Sprint(tc), func(t *testing.T) {
+			assert.Equal(t, tc.len, len(tc.in.AsMetricHTTP()))
+		})
+	}
+}
+
+func TestOtlpOptions_AsMetricGRPC(t *testing.T) {
+	type testCase struct {
+		in  otlpOptions
+		len int
+	}
+	testCases := []testCase{
+		{in: otlpOptions{Endpoint: "foo"}, len: 1},
+		{in: otlpOptions{Endpoint: "foo", Insecure: true}, len: 2},
+		{in: otlpOptions{Endpoint: "foo", SkipTLSVerify: true}, len: 2},
+		{in: otlpOptions{Endpoint: "foo", Insecure: true, SkipTLSVerify: true}, len: 3},
+	}
+	for _, tc := range testCases {
+		t.Run(fmt.Sprint(tc), func(t *testing.T) {
+			assert.Equal(t, tc.len, len(tc.in.AsMetricGRPC()))
+		})
+	}
+}
+
+func TestOtlpOptions_AsTraceHTTP(t *testing.T) {
+	type testCase struct {
+		in  otlpOptions
+		len int
+	}
+	testCases := []testCase{
+		{in: otlpOptions{Endpoint: "foo"}, len: 1},
+		{in: otlpOptions{Endpoint: "foo", Insecure: true}, len: 2},
+		{in: otlpOptions{Endpoint: "foo", URLPath: "/foo"}, len: 2},
+		{in: otlpOptions{Endpoint: "foo", SkipTLSVerify: true}, len: 2},
+		{in: otlpOptions{Endpoint: "foo", Insecure: true, SkipTLSVerify: true}, len: 3},
+		{in: otlpOptions{Endpoint: "foo", URLPath: "/foo", SkipTLSVerify: true}, len: 3},
+		{in: otlpOptions{Endpoint: "foo", URLPath: "/foo", Insecure: true, SkipTLSVerify: true}, len: 4},
+	}
+	for _, tc := range testCases {
+		t.Run(fmt.Sprint(tc), func(t *testing.T) {
+			assert.Equal(t, tc.len, len(tc.in.AsTraceHTTP()))
+		})
+	}
+}
+
+func TestOtlpOptions_AsTraceGRPC(t *testing.T) {
+	type testCase struct {
+		in  otlpOptions
+		len int
+	}
+	testCases := []testCase{
+		{in: otlpOptions{Endpoint: "foo"}, len: 1},
+		{in: otlpOptions{Endpoint: "foo", Insecure: true}, len: 2},
+		{in: otlpOptions{Endpoint: "foo", SkipTLSVerify: true}, len: 2},
+		{in: otlpOptions{Endpoint: "foo", Insecure: true, SkipTLSVerify: true}, len: 3},
+	}
+	for _, tc := range testCases {
+		t.Run(fmt.Sprint(tc), func(t *testing.T) {
+			assert.Equal(t, tc.len, len(tc.in.AsTraceGRPC()))
+		})
+	}
+}

pkg/internal/export/otel/metrics.go

@@ -2,7 +2,6 @@ package otel
 
 import (
 	"context"
-	"crypto/tls"
 	"fmt"
 	"net/url"
 	"os"
@@ -19,7 +18,6 @@ import (
 	"go.opentelemetry.io/otel/sdk/metric/aggregation"
 	semconv "go.opentelemetry.io/otel/semconv/v1.17.0"
 	"golang.org/x/exp/slog"
-	"google.golang.org/grpc/credentials"
 
 	"github.com/grafana/beyla/pkg/internal/imetrics"
 	"github.com/grafana/beyla/pkg/internal/pipe/global"
@@ -40,9 +38,10 @@ const (
 )
 
 type MetricsConfig struct {
-	Interval        time.Duration `yaml:"interval" env:"METRICS_INTERVAL"`
-	Endpoint        string        `yaml:"endpoint" env:"OTEL_EXPORTER_OTLP_ENDPOINT"`
-	MetricsEndpoint string        `yaml:"-" env:"OTEL_EXPORTER_OTLP_METRICS_ENDPOINT"`
+	Interval time.Duration `yaml:"interval" env:"METRICS_INTERVAL"`
+
+	CommonEndpoint  string `yaml:"-" env:"OTEL_EXPORTER_OTLP_ENDPOINT"`
+	MetricsEndpoint string `yaml:"endpoint" env:"OTEL_EXPORTER_OTLP_METRICS_ENDPOINT"`
 
 	Protocol        Protocol `yaml:"protocol" env:"OTEL_EXPORTER_OTLP_PROTOCOL"`
 	MetricsProtocol Protocol `yaml:"-" env:"OTEL_EXPORTER_OTLP_METRICS_PROTOCOL"`
@@ -75,7 +74,7 @@ func (m *MetricsConfig) GetProtocol() Protocol {
 // This method is invoked only once during startup time so it doesn't have a noticeable performance impact.
 // nolint:gocritic
 func (m MetricsConfig) Enabled() bool {
-	return m.Endpoint != "" || m.MetricsEndpoint != ""
+	return m.CommonEndpoint != "" || m.MetricsEndpoint != ""
 }
 
 // MetricsReporter implements the graph node that receives request.Span
@@ -215,7 +214,7 @@ func httpMetricsExporter(ctx context.Context, cfg *MetricsConfig) (metric.Export
 	if err != nil {
 		return nil, err
 	}
-	mexp, err := otlpmetrichttp.New(ctx, opts...)
+	mexp, err := otlpmetrichttp.New(ctx, opts.AsMetricHTTP()...)
 	if err != nil {
 		return nil, fmt.Errorf("creating HTTP metric exporter: %w", err)
 	}
@@ -227,7 +226,7 @@ func grpcMetricsExporter(ctx context.Context, cfg *MetricsConfig) (metric.Export
 	if err != nil {
 		return nil, err
 	}
-	mexp, err := otlpmetricgrpc.New(ctx, opts...)
+	mexp, err := otlpmetricgrpc.New(ctx, opts.AsMetricGRPC()...)
 	if err != nil {
 		return nil, fmt.Errorf("creating GRPC metric exporter: %w", err)
 	}
@@ -365,73 +364,80 @@ func (mr *MetricsReporter) reportMetrics(input <-chan []request.Span) {
 	mr.close()
 }
 
-func getHTTPMetricEndpointOptions(cfg *MetricsConfig) ([]otlpmetrichttp.Option, error) {
+func getHTTPMetricEndpointOptions(cfg *MetricsConfig) (otlpOptions, error) {
+	opts := otlpOptions{}
 	log := mlog().With("transport", "http")
-	murl, err := parseMetricsEndpoint(cfg)
+	murl, isCommon, err := parseMetricsEndpoint(cfg)
 	if err != nil {
-		return nil, err
+		return opts, err
 	}
 	log.Debug("Configuring exporter",
 		"protocol", cfg.Protocol, "metricsProtocol", cfg.MetricsProtocol, "endpoint", murl.Host)
 
 	setMetricsProtocol(cfg)
-	opts := []otlpmetrichttp.Option{
-		otlpmetrichttp.WithEndpoint(murl.Host),
-	}
+	opts.Endpoint = murl.Host
 	if murl.Scheme == "http" || murl.Scheme == "unix" {
 		log.Debug("Specifying insecure connection", "scheme", murl.Scheme)
-		opts = append(opts, otlpmetrichttp.WithInsecure())
-	}
-	if len(murl.Path) > 0 && murl.Path != "/" && !strings.HasSuffix(murl.Path, "/v1/metrics") {
-		urlPath := murl.Path + "/v1/metrics"
-		log.Debug("Specifying path", "path", urlPath)
-		opts = append(opts, otlpmetrichttp.WithURLPath(urlPath))
+		opts.Insecure = true
+	}
+	// If the value is set from the OTEL_EXPORTER_OTLP_ENDPOINT common property, we need to add /v1/metrics to the path
+	// otherwise, we leave the path that is explicitly set by the user
+	opts.URLPath = murl.Path
+	if isCommon {
+		if strings.HasSuffix(opts.URLPath, "/") {
+			opts.URLPath += "v1/metrics"
+		} else {
+			opts.URLPath += "/v1/metrics"
+		}
 	}
+	log.Debug("Specifying path", "path", opts.URLPath)
+
 	if cfg.InsecureSkipVerify {
 		log.Debug("Setting InsecureSkipVerify")
-		opts = append(opts, otlpmetrichttp.WithTLSClientConfig(&tls.Config{InsecureSkipVerify: true}))
+		opts.SkipTLSVerify = cfg.InsecureSkipVerify
 	}
 	return opts, nil
 }
 
-func getGRPCMetricEndpointOptions(cfg *MetricsConfig) ([]otlpmetricgrpc.Option, error) {
+func getGRPCMetricEndpointOptions(cfg *MetricsConfig) (otlpOptions, error) {
+	opts := otlpOptions{}
 	log := mlog().With("transport", "grpc")
-	murl, err := parseMetricsEndpoint(cfg)
+	murl, _, err := parseMetricsEndpoint(cfg)
 	if err != nil {
-		return nil, err
+		return opts, err
 	}
 	log.Debug("Configuring exporter",
 		"protocol", cfg.Protocol, "metricsProtocol", cfg.MetricsProtocol, "endpoint", murl.Host)
 
 	setMetricsProtocol(cfg)
-	opts := []otlpmetricgrpc.Option{
-		otlpmetricgrpc.WithEndpoint(murl.Host),
-	}
+	opts.Endpoint = murl.Host
 	if murl.Scheme == "http" || murl.Scheme == "unix" {
 		log.Debug("Specifying insecure connection", "scheme", murl.Scheme)
-		opts = append(opts, otlpmetricgrpc.WithInsecure())
+		opts.Insecure = true
 	}
 	if cfg.InsecureSkipVerify {
 		log.Debug("Setting InsecureSkipVerify")
-		opts = append(opts, otlpmetricgrpc.WithTLSCredentials(credentials.NewTLS(&tls.Config{InsecureSkipVerify: true})))
+		opts.SkipTLSVerify = true
 	}
 	return opts, nil
 }
 
-func parseMetricsEndpoint(cfg *MetricsConfig) (*url.URL, error) {
+func parseMetricsEndpoint(cfg *MetricsConfig) (*url.URL, bool, error) {
+	isCommon := false
 	endpoint := cfg.MetricsEndpoint
 	if endpoint == "" {
-		endpoint = cfg.Endpoint
+		isCommon = true
+		endpoint = cfg.CommonEndpoint
 	}
 
 	murl, err := url.Parse(endpoint)
 	if err != nil {
-		return nil, fmt.Errorf("parsing endpoint URL %s: %w", endpoint, err)
+		return nil, isCommon, fmt.Errorf("parsing endpoint URL %s: %w", endpoint, err)
 	}
 	if murl.Scheme == "" || murl.Host == "" {
-		return nil, fmt.Errorf("URL %q must have a scheme and a host", endpoint)
+		return nil, isCommon, fmt.Errorf("URL %q must have a scheme and a host", endpoint)
 	}
-	return murl, nil
+	return murl, isCommon, nil
 }
 
 // HACK: at the time of writing this, the otelpmetrichttp API does not support explicitly