OpenTelemetry: docs, config tweaks, no empty decision ID attributes (#4146)

1. I think an extra section on OT is warranted, even if it's brief.

2. The config now expects "type" to be set to "grpc" to enable the current
   feature set. More options may follow in the future.

This is meant to future-proof the config. Future values might include
"http", where the address, TLS and sampling related settings would be
reused; and "global", where the setup would use whatever global trace
provider was configured with otel. The latter use case would be for
embedding OPA via the SDK.

3. Decision ID attributes are only added to spans if decision logging is enabled.

Fixes #4128.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>

docs/content/configuration.md

@@ -815,7 +815,7 @@ Distributed tracing represents the configuration of the OpenTelemetry Tracing.
 
 | Field | Type | Required | Description |
 | --- | --- | --- | --- |
-| `distributed_tracing.enabled` | `bool` | No (default: `false`) | Enables distributed tracing. |
+| `distributed_tracing.type` | `string` | No | Setting this to "grpc" enables distributed tracing with an collector gRPC endpoint |
 | `distributed_tracing.address` | `string` | No (default: `localhost:4317`) | Address of the OpenTelemetry Collector gRPC endpoint. |
 | `distributed_tracing.service_name` | `string` | No (default: `opa`) | Logical name of the service. |
 | `distributed_tracing.sample_percentage` | `int` | No (default: `100`) | Percentage of traces that are sampled and exported. |

docs/content/monitoring.md

@@ -5,6 +5,22 @@ weight: 30
 restrictedtoc: true
 ---
 
+## OpenTelemetry
+
+When run as a server and configured accordingly, OPA will emit spans to an
+[OpenTelemetry](https://opentelemetry.io/) collector via gRPC.
+
+Each [REST API](../rest-api/) request sent to the server will start a span.
+If processing the request involves policy evaluation, and that in turn uses
+[`http.send`](../policy-reference/#http), those HTTP clients will emit descendant spans.
+
+Furthermore, spans exported for policy evaluation requests will contain an
+attribute `opa.decision_id` of the evaluation's decision ID _if_ the server
+has decision logging enabled.
+
+See [the configuration documentation](../configuration/#distributed-tracing)
+for all OpenTelemetry-related configurables.
+
 ## Prometheus
 
 OPA exposes an HTTP endpoint that can be used to collect performance metrics

internal/distributedtracing/distributedtracing.go

@@ -10,6 +10,7 @@ import (
 	"crypto/x509"
 	"fmt"
 	"io/ioutil"
+	"strings"
 
 	"github.com/go-logr/logr"
 	"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"
@@ -34,7 +35,6 @@ import (
 )
 
 const (
-	defaultEnable               = false
 	defaultAddress              = "localhost:4317"
 	defaultServiceName          = "opa"
 	defaultSampleRatePercentage = int(100)
@@ -56,7 +56,7 @@ func isSupportedSampleRatePercentage(sampleRate int) bool {
 }
 
 type distributedTracingConfig struct {
-	Enabled               *bool  `json:"enabled,omitempty"`
+	Type                  string `json:"type,omitempty"`
 	Address               string `json:"address,omitempty"`
 	ServiceName           string `json:"service_name,omitempty"`
 	SampleRatePercentage  *int   `json:"sample_percentage,omitempty"`
@@ -78,7 +78,7 @@ func Init(ctx context.Context, raw []byte, id string) (*otlptrace.Exporter, trac
 		return nil, nil, err
 	}
 
-	if !*distributedTracingConfig.Enabled {
+	if strings.ToLower(distributedTracingConfig.Type) != "grpc" {
 		return nil, nil, nil
 	}
 
@@ -132,13 +132,11 @@ func SetupLogging(logger logging.Logger) {
 
 func parseDistributedTracingConfig(raw []byte) (*distributedTracingConfig, error) {
 	if raw == nil {
-		enabled, encryptionSkipVerify := new(bool), new(bool)
+		encryptionSkipVerify := new(bool)
 		sampleRatePercentage := new(int)
-		*enabled = defaultEnable
 		*sampleRatePercentage = defaultSampleRatePercentage
 		*encryptionSkipVerify = defaultEncryptionSkipVerify
 		return &distributedTracingConfig{
-			Enabled:              enabled,
 			Address:              defaultAddress,
 			ServiceName:          defaultServiceName,
 			SampleRatePercentage: sampleRatePercentage,
@@ -148,31 +146,23 @@ func parseDistributedTracingConfig(raw []byte) (*distributedTracingConfig, error
 	}
 	var config distributedTracingConfig
 
-	if err := util.Unmarshal(raw, &config); err == nil {
-		if err = config.validateAndInjectDefaults(); err != nil {
-			return nil, err
-		}
-	} else {
+	if err := util.Unmarshal(raw, &config); err != nil {
 		return nil, err
 	}
-
-	if !isSupportedEncryptionScheme(config.EncryptionScheme) {
-		return nil, fmt.Errorf("unsupported distributed_tracing.encryption_scheme '%v'", config.EncryptionScheme)
-	}
-
-	if !isSupportedSampleRatePercentage(*config.SampleRatePercentage) {
-		return nil, fmt.Errorf("unsupported distributed_tracing.sample_percentage '%v'", *config.SampleRatePercentage)
+	if err := config.validateAndInjectDefaults(); err != nil {
+		return nil, err
 	}
 
 	return &config, nil
 }
 
 func (c *distributedTracingConfig) validateAndInjectDefaults() error {
-	if c.Enabled == nil {
-		enabled := new(bool)
-		*enabled = defaultEnable
-		c.Enabled = enabled
+	switch c.Type {
+	case "", "grpc": // OK
+	default:
+		return fmt.Errorf("unknown distributed_tracing.type '%s', must be \"grpc\" or \"\" (unset)", c.Type)
 	}
+
 	if c.Address == "" {
 		c.Address = defaultAddress
 	}
@@ -193,6 +183,14 @@ func (c *distributedTracingConfig) validateAndInjectDefaults() error {
 		c.EncryptionSkipVerify = encryptionSkipVerify
 	}
 
+	if !isSupportedEncryptionScheme(c.EncryptionScheme) {
+		return fmt.Errorf("unsupported distributed_tracing.encryption_scheme '%s'", c.EncryptionScheme)
+	}
+
+	if !isSupportedSampleRatePercentage(*c.SampleRatePercentage) {
+		return fmt.Errorf("unsupported distributed_tracing.sample_percentage '%v'", *c.SampleRatePercentage)
+	}
+
 	return nil
 }
 
@@ -204,7 +202,9 @@ func loadCertificate(tlsCertFile, tlsPrivateKeyFile string) (*tls.Certificate, e
 			return nil, err
 		}
 		return &cert, nil
-	} else if tlsCertFile != "" || tlsPrivateKeyFile != "" {
+	}
+
+	if tlsCertFile != "" || tlsPrivateKeyFile != "" {
 		return nil, fmt.Errorf("distributed_tracing.tls_cert_file and distributed_tracing.tls_private_key_file must be specified together")
 	}
 
@@ -212,20 +212,19 @@ func loadCertificate(tlsCertFile, tlsPrivateKeyFile string) (*tls.Certificate, e
 }
 
 func loadCertPool(tlsCACertFile string) (*x509.CertPool, error) {
-
-	if tlsCACertFile != "" {
-		caCertPEM, err := ioutil.ReadFile(tlsCACertFile)
-		if err != nil {
-			return nil, fmt.Errorf("read CA cert file: %v", err)
-		}
-		pool := x509.NewCertPool()
-		if ok := pool.AppendCertsFromPEM(caCertPEM); !ok {
-			return nil, fmt.Errorf("failed to parse CA cert %q", tlsCACertFile)
-		}
-		return pool, nil
+	if tlsCACertFile == "" {
+		return nil, nil
 	}
 
-	return nil, nil
+	caCertPEM, err := ioutil.ReadFile(tlsCACertFile)
+	if err != nil {
+		return nil, fmt.Errorf("read CA cert file: %v", err)
+	}
+	pool := x509.NewCertPool()
+	if ok := pool.AppendCertsFromPEM(caCertPEM); !ok {
+		return nil, fmt.Errorf("failed to parse CA cert %q", tlsCACertFile)
+	}
+	return pool, nil
 }
 
 func tlsOption(encryptionScheme string, encryptionSkipVerify bool, cert *tls.Certificate, certPool *x509.CertPool) (otlptracegrpc.Option, error) {

server/server.go

@@ -2865,6 +2865,9 @@ func parseURL(s string, useHTTPSByDefault bool) (*url.URL, error) {
 }
 
 func annotateSpan(ctx context.Context, decisionID string) {
+	if decisionID == "" {
+		return
+	}
 	trace.SpanFromContext(ctx).
 		SetAttributes(attribute.String(otelDecisionIDAttr, decisionID))
 }

server/server_test.go

@@ -4057,7 +4057,7 @@ func TestDiagnosticRoutes(t *testing.T) {
 
 func TestDistributedTracingEnabled(t *testing.T) {
 	c := []byte(`{"distributed_tracing": {
-		"enabled": true
+		"type": "grpc"
 		}}`)
 
 	ctx := context.Background()