From 0b1aec676727a5700bbce88240411d5df893bc83 Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Mon, 27 May 2024 14:21:40 +0200 Subject: [PATCH] Tracing: Various improvements (#88308) --- contribute/backend/instrumentation.md | 11 +++++----- pkg/infra/tracing/tracing.go | 22 +++++++++++++++++++ pkg/registry/apis/playlist/conversions.go | 2 +- pkg/services/encryption/service/helpers.go | 3 ++- pkg/services/encryption/service/service.go | 20 +++++++++++++++-- .../encryption/service/service_test.go | 5 +++-- pkg/services/secrets/manager/helpers.go | 4 +++- pkg/services/secrets/manager/manager.go | 10 +++++++++ pkg/services/secrets/manager/manager_test.go | 5 ++++- .../testdata/public_testdata.golden.jsonc | 8 +++---- 10 files changed, 72 insertions(+), 18 deletions(-) diff --git a/contribute/backend/instrumentation.md b/contribute/backend/instrumentation.md index d6d50c19c81..cebd0357837 100644 --- a/contribute/backend/instrumentation.md +++ b/contribute/backend/instrumentation.md @@ -194,12 +194,11 @@ func (s *MyService) Hello(ctx context.Context, name string) (string, error) { if name == "" { err := fmt.Errorf("name cannot be empty") - // sets the spanโ€™s status to Error to make the span tracking - // a failed operation as an error span. - span.SetStatus(codes.Error, "failed to check name") - // record err as an exception span event for this span - span.RecordError(err) - return "", err + // Use the helper functions tracing.Errorf or tracing.Error + // to set the spanโ€™s status to Error to make + // the span tracking a failed operation as an error span and + // record error as an exception span event for the provided span. + return "", tracing.Errorf(span, "failed to check name: %w", err) } // Add some other event to show Events usage diff --git a/pkg/infra/tracing/tracing.go b/pkg/infra/tracing/tracing.go index 73ad8801d55..1f0f7591957 100644 --- a/pkg/infra/tracing/tracing.go +++ b/pkg/infra/tracing/tracing.go @@ -2,6 +2,7 @@ package tracing import ( "context" + "errors" "fmt" "math" "net" @@ -14,6 +15,7 @@ import ( "go.opentelemetry.io/contrib/samplers/jaegerremote" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/codes" "go.opentelemetry.io/otel/exporters/jaeger" "go.opentelemetry.io/otel/exporters/otlp/otlptrace" "go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc" @@ -27,6 +29,7 @@ import ( "github.com/go-kit/log/level" "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/util/errutil" ) const ( @@ -109,6 +112,25 @@ func TraceIDFromContext(ctx context.Context, requireSampled bool) string { return spanCtx.TraceID().String() } +// Error sets the status to error and record the error as an exception in the provided span. +func Error(span trace.Span, err error) error { + attr := []attribute.KeyValue{} + grafanaErr := errutil.Error{} + if errors.As(err, &grafanaErr) { + attr = append(attr, attribute.String("message_id", grafanaErr.MessageID)) + } + + span.SetStatus(codes.Error, err.Error()) + span.RecordError(err, trace.WithAttributes(attr...)) + return err +} + +// Errorf wraps fmt.Errorf and also sets the status to error and record the error as an exception in the provided span. +func Errorf(span trace.Span, format string, args ...any) error { + err := fmt.Errorf(format, args...) + return Error(span, err) +} + type noopTracerProvider struct { trace.TracerProvider } diff --git a/pkg/registry/apis/playlist/conversions.go b/pkg/registry/apis/playlist/conversions.go index 59dc30e83da..632803970ab 100644 --- a/pkg/registry/apis/playlist/conversions.go +++ b/pkg/registry/apis/playlist/conversions.go @@ -91,7 +91,7 @@ func convertToK8sResource(v *playlistsvc.PlaylistDTO, namespacer request.Namespa if err == nil { meta.SetUpdatedTimestampMillis(v.UpdatedAt) if v.Id > 0 { - createdAt := time.UnixMilli(v.CreatedAt) + createdAt := time.UnixMilli(v.CreatedAt).UTC() meta.SetOriginInfo(&utils.ResourceOriginInfo{ Name: "SQL", Key: fmt.Sprintf("%d", v.Id), diff --git a/pkg/services/encryption/service/helpers.go b/pkg/services/encryption/service/helpers.go index 53f64239311..9575635cb5f 100644 --- a/pkg/services/encryption/service/helpers.go +++ b/pkg/services/encryption/service/helpers.go @@ -5,6 +5,7 @@ import ( "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/infra/usagestats" encryptionprovider "github.com/grafana/grafana/pkg/services/encryption/provider" "github.com/grafana/grafana/pkg/setting" @@ -17,7 +18,7 @@ func SetupTestService(tb testing.TB) *Service { provider := encryptionprovider.ProvideEncryptionProvider() settings := setting.NewCfg() - service, err := ProvideEncryptionService(provider, usMock, settings) + service, err := ProvideEncryptionService(tracing.InitializeTracerForTest(), provider, usMock, settings) require.NoError(tb, err) return service diff --git a/pkg/services/encryption/service/service.go b/pkg/services/encryption/service/service.go index a111e267407..e221252ad91 100644 --- a/pkg/services/encryption/service/service.go +++ b/pkg/services/encryption/service/service.go @@ -7,7 +7,10 @@ import ( "errors" "fmt" + "go.opentelemetry.io/otel/attribute" + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/infra/usagestats" "github.com/grafana/grafana/pkg/services/encryption" "github.com/grafana/grafana/pkg/setting" @@ -24,7 +27,8 @@ const ( // Service must not be used for encryption. // Use secrets.Service implementing envelope encryption instead. type Service struct { - log log.Logger + tracer tracing.Tracer + log log.Logger cfg *setting.Cfg usageMetrics usagestats.Service @@ -34,12 +38,14 @@ type Service struct { } func ProvideEncryptionService( + tracer tracing.Tracer, provider encryption.Provider, usageMetrics usagestats.Service, cfg *setting.Cfg, ) (*Service, error) { s := &Service{ - log: log.New("encryption"), + tracer: tracer, + log: log.New("encryption"), ciphers: provider.ProvideCiphers(), deciphers: provider.ProvideDeciphers(), @@ -93,6 +99,9 @@ func (s *Service) registerUsageMetrics() { } func (s *Service) Decrypt(ctx context.Context, payload []byte, secret string) ([]byte, error) { + ctx, span := s.tracer.Start(ctx, "encryption.service.Decrypt") + defer span.End() + var err error defer func() { if err != nil { @@ -115,6 +124,8 @@ func (s *Service) Decrypt(ctx context.Context, payload []byte, secret string) ([ return nil, err } + span.SetAttributes(attribute.String("encryption.algorithm", algorithm)) + var decrypted []byte decrypted, err = decipher.Decrypt(ctx, toDecrypt, secret) @@ -163,6 +174,9 @@ func (s *Service) deriveEncryptionAlgorithm(payload []byte) (string, []byte, err } func (s *Service) Encrypt(ctx context.Context, payload []byte, secret string) ([]byte, error) { + ctx, span := s.tracer.Start(ctx, "encryption.service.Encrypt") + defer span.End() + var err error defer func() { if err != nil { @@ -179,6 +193,8 @@ func (s *Service) Encrypt(ctx context.Context, payload []byte, secret string) ([ return nil, err } + span.SetAttributes(attribute.String("encryption.algorithm", algorithm)) + var encrypted []byte encrypted, err = cipher.Encrypt(ctx, payload, secret) diff --git a/pkg/services/encryption/service/service_test.go b/pkg/services/encryption/service/service_test.go index e785f8a3ab1..88a83a8b1aa 100644 --- a/pkg/services/encryption/service/service_test.go +++ b/pkg/services/encryption/service/service_test.go @@ -7,6 +7,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/infra/usagestats" "github.com/grafana/grafana/pkg/services/encryption" "github.com/grafana/grafana/pkg/services/encryption/provider" @@ -20,7 +21,7 @@ func Test_Service(t *testing.T) { usageStats := &usagestats.UsageStatsMock{} settings := setting.NewCfg() - svc, err := ProvideEncryptionService(encProvider, usageStats, settings) + svc, err := ProvideEncryptionService(tracing.InitializeTracerForTest(), encProvider, usageStats, settings) require.NoError(t, err) t.Run("decrypt empty payload should return error", func(t *testing.T) { @@ -79,7 +80,7 @@ func Test_Service_MissingProvider(t *testing.T) { usageStats := &usagestats.UsageStatsMock{} settings := setting.NewCfg() - service, err := ProvideEncryptionService(encProvider, usageStats, settings) + service, err := ProvideEncryptionService(tracing.InitializeTracerForTest(), encProvider, usageStats, settings) assert.Nil(t, service) assert.Error(t, err) } diff --git a/pkg/services/secrets/manager/helpers.go b/pkg/services/secrets/manager/helpers.go index 9633bde3508..5c6249a8e1c 100644 --- a/pkg/services/secrets/manager/helpers.go +++ b/pkg/services/secrets/manager/helpers.go @@ -6,6 +6,7 @@ import ( "github.com/stretchr/testify/require" "gopkg.in/ini.v1" + "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/infra/usagestats" encryptionprovider "github.com/grafana/grafana/pkg/services/encryption/provider" encryptionservice "github.com/grafana/grafana/pkg/services/encryption/service" @@ -40,10 +41,11 @@ func setupTestService(tb testing.TB, store secrets.Store, features featuremgmt.F encProvider := encryptionprovider.Provider{} usageStats := &usagestats.UsageStatsMock{} - encryption, err := encryptionservice.ProvideEncryptionService(encProvider, usageStats, cfg) + encryption, err := encryptionservice.ProvideEncryptionService(tracing.InitializeTracerForTest(), encProvider, usageStats, cfg) require.NoError(tb, err) secretsService, err := ProvideSecretsService( + tracing.InitializeTracerForTest(), store, osskmsproviders.ProvideService(encryption, cfg, features), encryption, diff --git a/pkg/services/secrets/manager/manager.go b/pkg/services/secrets/manager/manager.go index eb8179ece19..d1b200c7d46 100644 --- a/pkg/services/secrets/manager/manager.go +++ b/pkg/services/secrets/manager/manager.go @@ -15,6 +15,7 @@ import ( "golang.org/x/sync/errgroup" "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/infra/usagestats" "github.com/grafana/grafana/pkg/services/encryption" "github.com/grafana/grafana/pkg/services/featuremgmt" @@ -35,6 +36,7 @@ var ( ) type SecretsService struct { + tracer tracing.Tracer store secrets.Store enc encryption.Internal cfg *setting.Cfg @@ -54,6 +56,7 @@ type SecretsService struct { } func ProvideSecretsService( + tracer tracing.Tracer, store secrets.Store, kmsProvidersService kmsproviders.Service, enc encryption.Internal, @@ -68,6 +71,7 @@ func ProvideSecretsService( )) s := &SecretsService{ + tracer: tracer, store: store, enc: enc, cfg: cfg, @@ -158,6 +162,9 @@ func (s *SecretsService) encryptedWithEnvelopeEncryption(payload []byte) bool { var b64 = base64.RawStdEncoding func (s *SecretsService) Encrypt(ctx context.Context, payload []byte, opt secrets.EncryptionOptions) ([]byte, error) { + ctx, span := s.tracer.Start(ctx, "secretsService.Encrypt") + defer span.End() + // Use legacy encryption service if featuremgmt.FlagDisableEnvelopeEncryption toggle is on if s.features.IsEnabled(ctx, featuremgmt.FlagDisableEnvelopeEncryption) { return s.enc.Encrypt(ctx, payload, s.cfg.SecretKey) @@ -313,6 +320,9 @@ func newRandomDataKey() ([]byte, error) { } func (s *SecretsService) Decrypt(ctx context.Context, payload []byte) ([]byte, error) { + ctx, span := s.tracer.Start(ctx, "secretsService.Decrypt") + defer span.End() + var err error defer func() { opsCounter.With(prometheus.Labels{ diff --git a/pkg/services/secrets/manager/manager_test.go b/pkg/services/secrets/manager/manager_test.go index c5e23232df6..3efaef05e51 100644 --- a/pkg/services/secrets/manager/manager_test.go +++ b/pkg/services/secrets/manager/manager_test.go @@ -11,6 +11,7 @@ import ( "gopkg.in/ini.v1" "github.com/grafana/grafana/pkg/infra/db" + "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/infra/usagestats" encryptionprovider "github.com/grafana/grafana/pkg/services/encryption/provider" encryptionservice "github.com/grafana/grafana/pkg/services/encryption/service" @@ -192,7 +193,7 @@ func TestSecretsService_UseCurrentProvider(t *testing.T) { encProvider := encryptionprovider.Provider{} usageStats := &usagestats.UsageStatsMock{} - encryptionService, err := encryptionservice.ProvideEncryptionService(encProvider, usageStats, cfg) + encryptionService, err := encryptionservice.ProvideEncryptionService(tracing.InitializeTracerForTest(), encProvider, usageStats, cfg) require.NoError(t, err) features := featuremgmt.WithFeatures() @@ -201,6 +202,7 @@ func TestSecretsService_UseCurrentProvider(t *testing.T) { secretStore := database.ProvideSecretsStore(testDB) secretsService, err := ProvideSecretsService( + tracing.InitializeTracerForTest(), secretStore, &kms, encryptionService, @@ -219,6 +221,7 @@ func TestSecretsService_UseCurrentProvider(t *testing.T) { // secret service tries to find a DEK in a cache first before calling provider's decrypt // to bypass the cache, we set up one more secrets service to test decrypting svcDecrypt, err := ProvideSecretsService( + tracing.InitializeTracerForTest(), secretStore, &kms, encryptionService, diff --git a/pkg/services/store/testdata/public_testdata.golden.jsonc b/pkg/services/store/testdata/public_testdata.golden.jsonc index a37239596c2..88d0760e029 100644 --- a/pkg/services/store/testdata/public_testdata.golden.jsonc +++ b/pkg/services/store/testdata/public_testdata.golden.jsonc @@ -1,5 +1,5 @@ // ๐ŸŒŸ This was machine generated. Do not edit. ๐ŸŒŸ -// +// // Frame[0] { // "type": "directory-listing", // "typeVersion": [ @@ -10,7 +10,7 @@ // "HasMore": false // } // } -// Name: +// Name: // Dimensions: 3 Fields by 3 Rows // +----------------------------+----------------------+---------------+ // | Name: name | Name: mediaType | Name: size | @@ -21,8 +21,8 @@ // | example-with-style.geojson | application/geo+json | 3332 | // | usa-states.geojson | application/geo+json | 89263 | // +----------------------------+----------------------+---------------+ -// -// +// +// // ๐ŸŒŸ This was machine generated. Do not edit. ๐ŸŒŸ { "status": 200,