From 668708d5e71b64c74ac37e664ed2e32d490054f4 Mon Sep 17 00:00:00 2001 From: Stephan Renatus Date: Thu, 16 Dec 2021 20:05:18 +0100 Subject: [PATCH] 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 --- docs/content/configuration.md | 2 +- docs/content/monitoring.md | 16 +++++ .../distributedtracing/distributedtracing.go | 69 +++++++++---------- server/server.go | 3 + server/server_test.go | 2 +- 5 files changed, 55 insertions(+), 37 deletions(-) diff --git a/docs/content/configuration.md b/docs/content/configuration.md index 47439130ab..db7346ddb2 100644 --- a/docs/content/configuration.md +++ b/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. | diff --git a/docs/content/monitoring.md b/docs/content/monitoring.md index a61d3f1a68..2359d2a956 100644 --- a/docs/content/monitoring.md +++ b/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 diff --git a/internal/distributedtracing/distributedtracing.go b/internal/distributedtracing/distributedtracing.go index 71f73b61dd..3af532f20e 100644 --- a/internal/distributedtracing/distributedtracing.go +++ b/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) { diff --git a/server/server.go b/server/server.go index 396c154f58..846ab30a55 100644 --- a/server/server.go +++ b/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)) } diff --git a/server/server_test.go b/server/server_test.go index 550af1d906..8a5012d962 100644 --- a/server/server_test.go +++ b/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()