normalize attributes in different spans and update todos

Author: dhontecillas

doc/TODO.md

@@ -1,15 +1,44 @@
 # TODO
 
+- **we are reporting the size and time in `io.instruments.go` as both a counter and a histogram**
+  (count can be extracted from histogram in all "backends": prometheus, datadog, new relic, etc..?)
+
 - allow different formats for the propagation of the trace (the `TextMapPropagator`)
     in [endpoint.go](../router/gin/endpoint.go).
+
 - review how we pass the state.
 
 - we cannot use `skipPaths` with the same value that we have to define the endpoints
     at the global layer, because we cannot know the matched pattern
 
 - clean shutdown
 
+- add configuration options to allow to use grpc credentials
+
+- allow to tweak the `bucket` limits for different histograms (like 
+    latency and size) 
+    - `http/client/transport_metrics.go`: `timeBucketsOpt`, `sizeBucketsOpt`
+    - `http/server/metrics.go`: `timeBucketsOpt`, `sizeBucketsOpt`
+    - `io/instrumens.go`: `timeBucketsOpt`, `sizeBucketsOpt`
+
 # TO DECIDE
 
 - do we want to have the `service_name` to override the global ServiceConfig name for
     the KrakenD gateway ?
+
+- in `exporter/prometheus/prometheus.go` we could add a config option that will
+    add a prefix to all reported metrics (using the `WithNamespace` option).
+
+- in `lura/backend.go` and `lura/proxy.go` we are setting the static attrs, 
+  and using `semconv.ServerAddress`, to set the concatenated list of hosts.
+  Is that something that we want ? 
+  
+# TO CHECK
+
+- in `exporter/otelcollector/otelcollector.go`, the `WithEndpoint` states that 
+  **no http** schema should be  in cluded (nor path). To use `http` reporting methods, 
+  the `WithInsecure` option should be used (and to user a path `WithURLPath`).
+  Check that current implementation works as expected, or fix it.
+
+- in `http/client/transport.go` we have commented out the `StartOptions` for 
+    trace: review if would be useful to expose that in the config.

exporter/otelcollector/otelcollector.go

@@ -61,7 +61,6 @@ func Exporter(ctx context.Context, cfg config.OTLPExporter) (*OtelCollector, err
 			// WARN: we do not clean up / canonicalize the header name
 			headers[kvh.Key] = kvh.Value
 		}
-		// TODO: allow to use http in config
 		exporter, err = otlptracehttp.New(ctx,
 			otlptracehttp.WithHeaders(headers),
 			otlptracehttp.WithEndpoint(fmt.Sprintf("%s:%d", cfg.Host, cfg.Port)),

exporter/prometheus/prometheus.go

@@ -53,7 +53,6 @@ func Exporter(ctx context.Context, cfg config.PrometheusExporter) (*PrometheusCo
 		}
 	}
 
-	// TODO: should we put a WithNamespace option here ?
 	exporter, err := prometheus.New(prometheus.WithRegisterer(prometheusRegistry))
 	if err != nil {
 		return nil, err

http/client/client.go

@@ -40,12 +40,10 @@ import (
 )
 
 // InstrumentedHTTPClient creates a new instrumented http client with the options provided.
-// If the provided options are nil, the default options (with everything enabled) will be used.
+// If the provided options are nil, the default options (with everything enabled, except
+// the detailed connection data: DNS, TLS, time to get a connection...) will be used.
 // Check the [TransportOptions] struct for details..
 func InstrumentedHTTPClient(c *http.Client, t *TransportOptions, clientName string) *http.Client {
-	// the default options is to have everything activated
-	// TODO: we might want to return the provided client if no transport options are provided ?
-	// TODO: we might want to return an error if no transport options are provided ?
 	opts := TransportOptions{
 		MetricsOpts: TransportMetricsOptions{
 			RoundTrip:          true,

http/client/transport.go

@@ -56,7 +56,6 @@ type Transport struct {
 	//
 	// StartOptions.SpanKind will always be set to trace.SpanKindClient
 	// for spans started by this transport.
-	// TODO:
 	// StartOptions trace.StartOptions
 
 	tracesOpts      TransportTracesOptions

http/client/transport_metrics.go

@@ -10,7 +10,6 @@ import (
 )
 
 var (
-	// TODO: we can tweak the level of detail for the time buckets:
 	// this will generate 20 time series for the metric
 	timeBucketsOpt = metric.WithExplicitBucketBoundaries(
 		0.010, 0.020, 0.030, 0.040,

http/server/metrics.go

@@ -6,13 +6,7 @@ import (
 	"go.opentelemetry.io/otel/semconv/v1.21.0"
 )
 
-// TODO: we must decide if we want a single shared configuration of the
-// time buckets, and the size buckets, or if we want to be able to tweak
-// the buckets for each layer (or give some options, like low / medium / high
-// detail that would add more or less buckets).
 var (
-	// TODO: we can tweak the level of detail for the time buckets:
-	// this will generate 20 time series for the metric
 	timeBucketsOpt = metric.WithExplicitBucketBoundaries(
 		0.010, 0.020, 0.030, 0.040,
 		0.050, 0.065, 0.080, 0.100,

http/server/server.go

@@ -64,8 +64,6 @@ func NewTrackingHandler(next http.Handler, obsCfg *kotelconfig.Config, stateFn s
 
 	var m *metricsHTTP
 	if !gCfg.DisableMetrics {
-		// TODO: we might want to create a single "meter" in the state instead of using
-		// meter provider
 		meter := s.Meter()
 		staticAttrs := []attribute.KeyValue{
 			attribute.String("krakend.stage", "global"),

io/instruments.go

@@ -41,9 +41,6 @@ type instruments struct {
 	metricFixedAttrs []attribute.KeyValue
 	traceFixedAttrs  []attribute.KeyValue
 
-	//  Metrics:
-	// the count of number of bytes sent (TODO: we might remove
-	// this, because that count can be extracted from the histogram)
 	sizeCount     metric.Int64Counter
 	sizeHistogram metric.Int64Histogram
 	timeCount     metric.Float64Counter

lura/attributes.go

@@ -0,0 +1,56 @@
+package lura
+
+import (
+	"sort"
+	"strings"
+
+	"go.opentelemetry.io/otel/attribute"
+	"go.opentelemetry.io/otel/semconv/v1.21.0"
+
+	"github.com/luraproject/lura/v2/config"
+
+	kotelconfig "github.com/krakend/krakend-otel/config"
+)
+
+// backendConfigAttributes returnsa list of attributes
+// that will be set for both traces and
+// metrics, as those are expected to have low cardinality
+//   - the method: one of the `GET`, `POST`, `PUT` .. etc
+//   - the "path" , that is actually the path "template" to not have different values
+//     for different params but the same endpoint.
+//   - the krakend stage, that can be one of
+//   - router: includes from the very point of receiving a request until
+//     a response is returned to the client.
+//   - pipe: includes all the processing that is performed
+//     for the endpoint part of a request (like merging and grouping
+//     responses from different backends).
+//   - backend: includes all middlewares and processing that is done for
+//     a given backend.
+//   - backend-request: when reporting the request to the backends
+//   - server address: the host for the request
+func backendConfigAttributes(cfg *config.Backend, stage string) []attribute.KeyValue {
+
+	urlPattern := kotelconfig.NormalizeURLPattern(cfg.URLPattern)
+	parentEndpoint := kotelconfig.NormalizeURLPattern(cfg.ParentEndpoint)
+
+	attrs := []attribute.KeyValue{
+		semconv.HTTPRequestMethodOriginal(cfg.Method),
+		semconv.URLPath(urlPattern), // <- for traces we can use URLFull to not have the matched path
+		attribute.String("krakend.stage", stage),
+		attribute.String("krakend.endpoint", parentEndpoint),
+	}
+	numHosts := len(cfg.Host)
+	if numHosts > 0 {
+		if numHosts == 1 {
+			attrs = append(attrs, semconv.ServerAddress(cfg.Host[0]))
+		} else {
+			hosts := make([]string, 0, numHosts)
+			copy(hosts, cfg.Host)
+			sort.StringSlice(hosts).Sort()
+			strHosts := strings.Join(hosts, "_")
+			attrs = append(attrs, semconv.ServerAddress(strHosts))
+		}
+	}
+
+	return attrs
+}

lura/backend.go

@@ -5,7 +5,6 @@ import (
 	"net/http"
 
 	"go.opentelemetry.io/otel/attribute"
-	"go.opentelemetry.io/otel/semconv/v1.21.0"
 
 	"github.com/luraproject/lura/v2/config"
 	transport "github.com/luraproject/lura/v2/transport/http/client"
@@ -21,16 +20,14 @@ var defaultOpts = otelconfig.BackendOpts{
 		RoundTrip:          true,
 		ReadPayload:        true,
 		DetailedConnection: true,
-		// TODO: THIS IS NOT BEING USED !!
-		StaticAttributes: make(otelconfig.Attributes, 0),
+		StaticAttributes:   make(otelconfig.Attributes, 0),
 	},
 	Traces: &otelconfig.BackendTraceOpts{
 		DisableStage:       false,
 		RoundTrip:          true,
 		ReadPayload:        true,
 		DetailedConnection: true,
-		// TODO: THIS IS NOT BEING USED !!
-		StaticAttributes: make(otelconfig.Attributes, 0),
+		StaticAttributes:   make(otelconfig.Attributes, 0),
 	},
 }
 
@@ -86,38 +83,39 @@ func InstrumentedHTTPClientFactory(clientFactory transport.HTTPClientFactory,
 	//      - backend: includes all middlewares and processing that is done for
 	//          a given backend.
 	//      - backend-request: when reporting the request to the backends
-	attrs := []attribute.KeyValue{
-		semconv.HTTPRequestMethodOriginal(cfg.Method),
-		semconv.URLPath(urlPattern), // <- for traces we can use URLFull to not have the matched path
-		attribute.String("krakend.stage", "backend-request"),
-	}
+	attrs := backendConfigAttributes(cfg, "backend-request")
 
-	if len(cfg.Host) > 0 {
-		attrs = append(attrs, semconv.ServerAddress(cfg.Host[0]))
+	metricAttrs := attrs
+	if len(opts.Metrics.StaticAttributes) > 0 {
+		for _, kv := range opts.Metrics.StaticAttributes {
+			metricAttrs = append(metricAttrs, attribute.String(kv.Key, kv.Value))
+		}
+	}
+	traceAttrs := attrs
+	if len(opts.Traces.StaticAttributes) > 0 {
+		for _, kv := range opts.Metrics.StaticAttributes {
+			traceAttrs = append(traceAttrs, attribute.String(kv.Key, kv.Value))
+		}
 	}
-
-	// TODO: check how we want to deal with this "clientName"
-	clientName := cfg.ParentEndpoint + "." + urlPattern
-
 	t := clienthttp.TransportOptions{
 		MetricsOpts: clienthttp.TransportMetricsOptions{
 			RoundTrip:          opts.Metrics.RoundTrip,
 			ReadPayload:        opts.Metrics.ReadPayload,
 			DetailedConnection: opts.Metrics.DetailedConnection,
-			FixedAttributes:    attrs,
+			FixedAttributes:    metricAttrs,
 		},
 		TracesOpts: clienthttp.TransportTracesOptions{
 			RoundTrip:          opts.Traces.RoundTrip,
 			ReadPayload:        opts.Traces.ReadPayload,
 			DetailedConnection: opts.Traces.DetailedConnection,
-			FixedAttributes:    attrs,
+			FixedAttributes:    traceAttrs,
 		},
 		OTELInstance: getState,
 	}
 
 	return func(ctx context.Context) *http.Client {
 		// TODO: this can be optimized, because inside InstrumentedHTTPClient we
 		// are creating a RoundTripper that could be "prebuilt" and reused
-		return clienthttp.InstrumentedHTTPClient(clientFactory(ctx), &t, clientName)
+		return clienthttp.InstrumentedHTTPClient(clientFactory(ctx), &t, urlPattern)
 	}
 }

lura/proxy.go

@@ -3,7 +3,6 @@ package lura
 import (
 	"context"
 	"errors"
-	"strings"
 	"time"
 
 	"go.opentelemetry.io/otel/attribute"
@@ -187,18 +186,9 @@ func OTELBackendFactory(bf proxy.BackendFactory, gsfn state.GetterFn, metricsEna
 				return next
 			}
 		}
+		staticAttrs := backendConfigAttributes(cfg, "backend")
+
 		urlPattern := kotelconfig.NormalizeURLPattern(cfg.URLPattern)
-		parentEndpoint := kotelconfig.NormalizeURLPattern(cfg.ParentEndpoint)
-		hosts := strings.Join(cfg.Host, "_")
-		// TODO: check if we want to have the hosts as static attribute
-		staticAttrs := []attribute.KeyValue{
-			semconv.URLPath(urlPattern),
-			attribute.String("krakend.stage", "backend"),
-			attribute.String("krakend.endpoint", parentEndpoint),
-			semconv.ServerAddress(hosts),
-		}
-		// TODO: check if we want to have the hosts, to paths that are the same, but for different hosts
-		spanName := strings.Join(cfg.Host, "_") + urlPattern
-		return Middleware(staticAttrs, gsfn, metricsEnabled, tracesEnabled, spanName)(next)
+		return Middleware(staticAttrs, gsfn, metricsEnabled, tracesEnabled, urlPattern)(next)
 	}
 }