From 57be4275f0c9f4c026a20496e4877022783edaf1 Mon Sep 17 00:00:00 2001 From: Denise Li Date: Mon, 19 Aug 2024 20:16:18 -0400 Subject: [PATCH] feat: un-revert registration of `db.sql` metrics (#2373) Fixes https://github.com/TBD54566975/ftl/issues/2285 Unreverts https://github.com/TBD54566975/ftl/pull/2305 with these changes: * Reverts [`copyAny`](https://github.com/TBD54566975/ftl/blob/main/internal/reflect/reflect.go#L149) changes except for the `list.List` handling. * The original PR caused a panic when deploying to prod: `reflect: internal error: must be a Pointer or Ptr; got unsafe.Pointer` * https://github.com/TBD54566975/ftl/pull/2376 removed the DB handles from the module context Database struct, so we no longer need to handle all these types in `DeepCopy`. * Updates all `reflect.Ptr` refs to `reflect.Pointer`. Not a big deal, but `Ptr` is the [old name](https://cs.opensource.google/go/go/+/refs/tags/go1.23.0:src/reflect/type.go;l=303), and since we're using `reflect.Pointer` elsewhere in this file, it's better to stay consistent. Accordingly, this also fixes the error message that we saw in the prod deployment, which previously implied `Pointer` and `Ptr` were two _separate_ types. --------- Co-authored-by: github-actions[bot] --- .../controller/sql/databasetesting/devel.go | 5 ++-- cmd/ftl-controller/main.go | 3 +-- cmd/ftl/cmd_box_run.go | 4 ++-- cmd/ftl/cmd_serve.go | 3 +-- examples/go/echo/go.sum | 2 ++ .../testdata/go/runtimereflection/go.mod | 1 + .../testdata/go/runtimereflection/go.sum | 4 ++++ go.mod | 4 +++- go.sum | 2 ++ internal/observability/client_test.go | 2 +- internal/observability/db.go | 23 ++++++++++++++++++ internal/reflect/reflect.go | 24 ++++++++++++++++--- internal/reflect/reflect_test.go | 11 +++++++++ 13 files changed, 75 insertions(+), 13 deletions(-) create mode 100644 internal/observability/db.go diff --git a/backend/controller/sql/databasetesting/devel.go b/backend/controller/sql/databasetesting/devel.go index a52f3fb4c2..3be0b0d232 100644 --- a/backend/controller/sql/databasetesting/devel.go +++ b/backend/controller/sql/databasetesting/devel.go @@ -12,6 +12,7 @@ import ( "github.com/TBD54566975/ftl/backend/controller/sql" "github.com/TBD54566975/ftl/internal/log" + "github.com/TBD54566975/ftl/internal/observability" ) // CreateForDevel creates and migrates a new database for development or testing. @@ -29,7 +30,7 @@ func CreateForDevel(ctx context.Context, dsn string, recreate bool) (*stdsql.DB, var conn *stdsql.DB for range 10 { - conn, err = stdsql.Open("pgx", noDBDSN.String()) + conn, err = observability.OpenDBAndInstrument(noDBDSN.String()) if err == nil { defer conn.Close() break @@ -72,7 +73,7 @@ func CreateForDevel(ctx context.Context, dsn string, recreate bool) (*stdsql.DB, return nil, err } - realConn, err := stdsql.Open("pgx", dsn) + realConn, err := observability.OpenDBAndInstrument(dsn) if err != nil { return nil, fmt.Errorf("failed to open database: %w", err) } diff --git a/cmd/ftl-controller/main.go b/cmd/ftl-controller/main.go index 4ab85995fb..fda7db9e4c 100644 --- a/cmd/ftl-controller/main.go +++ b/cmd/ftl-controller/main.go @@ -2,7 +2,6 @@ package main import ( "context" - "database/sql" "fmt" "os" "strconv" @@ -54,7 +53,7 @@ func main() { kctx.FatalIfErrorf(err, "failed to initialize observability") // The FTL controller currently only supports DB as a configuration provider/resolver. - conn, err := sql.Open("pgx", cli.ControllerConfig.DSN) + conn, err := observability.OpenDBAndInstrument(cli.ControllerConfig.DSN) kctx.FatalIfErrorf(err) encryptionBuilder := encryption.NewBuilder().WithKMSURI(optional.Ptr(cli.ControllerConfig.KMSURI)) diff --git a/cmd/ftl/cmd_box_run.go b/cmd/ftl/cmd_box_run.go index c247750179..756d8fdcbd 100644 --- a/cmd/ftl/cmd_box_run.go +++ b/cmd/ftl/cmd_box_run.go @@ -2,7 +2,6 @@ package main import ( "context" - "database/sql" "fmt" "net/url" "time" @@ -20,6 +19,7 @@ import ( "github.com/TBD54566975/ftl/internal/buildengine" "github.com/TBD54566975/ftl/internal/log" "github.com/TBD54566975/ftl/internal/model" + "github.com/TBD54566975/ftl/internal/observability" "github.com/TBD54566975/ftl/internal/projectconfig" "github.com/TBD54566975/ftl/internal/rpc" ) @@ -58,7 +58,7 @@ func (b *boxRunCmd) Run(ctx context.Context, projConfig projectconfig.Config) er } // Bring up the DB connection and DAL. - conn, err := sql.Open("pgx", config.DSN) + conn, err := observability.OpenDBAndInstrument(config.DSN) if err != nil { return fmt.Errorf("failed to bring up DB connection: %w", err) } diff --git a/cmd/ftl/cmd_serve.go b/cmd/ftl/cmd_serve.go index 60f32316fc..e831bae96c 100644 --- a/cmd/ftl/cmd_serve.go +++ b/cmd/ftl/cmd_serve.go @@ -2,7 +2,6 @@ package main import ( "context" - "database/sql" "errors" "fmt" "net" @@ -148,7 +147,7 @@ func (s *serveCmd) run(ctx context.Context, projConfig projectconfig.Config, ini controllerCtx = cf.ContextWithSecrets(controllerCtx, sm) // Bring up the DB connection and DAL. - conn, err := sql.Open("pgx", config.DSN) + conn, err := observability.OpenDBAndInstrument(config.DSN) if err != nil { return fmt.Errorf("failed to bring up DB connection: %w", err) } diff --git a/examples/go/echo/go.sum b/examples/go/echo/go.sum index a0bde06e0d..34b6a7ddd4 100644 --- a/examples/go/echo/go.sum +++ b/examples/go/echo/go.sum @@ -134,6 +134,8 @@ gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8 gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +gotest.tools/v3 v3.5.1 h1:EENdUnS3pdur5nybKYIh2Vfgc8IUNBjxDPSjtiJcOzU= +gotest.tools/v3 v3.5.1/go.mod h1:isy3WKz7GK6uNw/sbHzfKBLvlvXwUyV06n6brMxxopU= modernc.org/gc/v3 v3.0.0-20240107210532-573471604cb6 h1:5D53IMaUuA5InSeMu9eJtlQXS2NxAhyWQvkKEgXZhHI= modernc.org/gc/v3 v3.0.0-20240107210532-573471604cb6/go.mod h1:Qz0X07sNOR1jWYCrJMEnbW/X55x206Q7Vt4mz6/wHp4= modernc.org/libc v1.55.3 h1:AzcW1mhlPNrRtjS5sS+eW2ISCgSOLLNyFzRh/V3Qj/U= diff --git a/go-runtime/ftl/reflection/testdata/go/runtimereflection/go.mod b/go-runtime/ftl/reflection/testdata/go/runtimereflection/go.mod index 2ce8c7e458..a60a25e2fe 100644 --- a/go-runtime/ftl/reflection/testdata/go/runtimereflection/go.mod +++ b/go-runtime/ftl/reflection/testdata/go/runtimereflection/go.mod @@ -11,6 +11,7 @@ require ( connectrpc.com/connect v1.16.2 // indirect connectrpc.com/grpcreflect v1.2.0 // indirect connectrpc.com/otelconnect v0.7.1 // indirect + github.com/XSAM/otelsql v0.32.0 // indirect github.com/alecthomas/atomic v0.1.0-alpha2 // indirect github.com/alecthomas/concurrency v0.0.2 // indirect github.com/alecthomas/participle/v2 v2.1.1 // indirect diff --git a/go-runtime/ftl/reflection/testdata/go/runtimereflection/go.sum b/go-runtime/ftl/reflection/testdata/go/runtimereflection/go.sum index a0bde06e0d..9423bf5ab0 100644 --- a/go-runtime/ftl/reflection/testdata/go/runtimereflection/go.sum +++ b/go-runtime/ftl/reflection/testdata/go/runtimereflection/go.sum @@ -6,6 +6,8 @@ connectrpc.com/otelconnect v0.7.1 h1:scO5pOb0i4yUE66CnNrHeK1x51yq0bE0ehPg6WvzXJY connectrpc.com/otelconnect v0.7.1/go.mod h1:dh3bFgHBTb2bkqGCeVVOtHJreSns7uu9wwL2Tbz17ms= github.com/TBD54566975/scaffolder v1.0.0 h1:QUFSy2wVzumLDg7IHcKC6AP+IYyqWe9Wxiu72nZn5qU= github.com/TBD54566975/scaffolder v1.0.0/go.mod h1:auVpczIbOAdIhYDVSruIw41DanxOKB9bSvjf6MEl7Fs= +github.com/XSAM/otelsql v0.32.0 h1:vDRE4nole0iOOlTaC/Bn6ti7VowzgxK39n3Ll1Kt7i0= +github.com/XSAM/otelsql v0.32.0/go.mod h1:Ary0hlyVBbaSwo8atZB8Aoothg9s/LBJj/N/p5qDmLM= github.com/alecthomas/assert/v2 v2.10.0 h1:jjRCHsj6hBJhkmhznrCzoNpbA3zqy0fYiUcYZP/GkPY= github.com/alecthomas/assert/v2 v2.10.0/go.mod h1:Bze95FyfUr7x34QZrjL+XP+0qgp/zg8yS+TtBj1WA3k= github.com/alecthomas/atomic v0.1.0-alpha2 h1:dqwXmax66gXvHhsOS4pGPZKqYOlTkapELkLb3MNdlH8= @@ -134,6 +136,8 @@ gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8 gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +gotest.tools/v3 v3.5.1 h1:EENdUnS3pdur5nybKYIh2Vfgc8IUNBjxDPSjtiJcOzU= +gotest.tools/v3 v3.5.1/go.mod h1:isy3WKz7GK6uNw/sbHzfKBLvlvXwUyV06n6brMxxopU= modernc.org/gc/v3 v3.0.0-20240107210532-573471604cb6 h1:5D53IMaUuA5InSeMu9eJtlQXS2NxAhyWQvkKEgXZhHI= modernc.org/gc/v3 v3.0.0-20240107210532-573471604cb6/go.mod h1:Qz0X07sNOR1jWYCrJMEnbW/X55x206Q7Vt4mz6/wHp4= modernc.org/libc v1.55.3 h1:AzcW1mhlPNrRtjS5sS+eW2ISCgSOLLNyFzRh/V3Qj/U= diff --git a/go.mod b/go.mod index fc046001d5..1e70a43dbd 100644 --- a/go.mod +++ b/go.mod @@ -9,6 +9,7 @@ require ( github.com/BurntSushi/toml v1.4.0 github.com/TBD54566975/golang-tools v0.2.1 github.com/TBD54566975/scaffolder v1.0.0 + github.com/XSAM/otelsql v0.32.0 github.com/alecthomas/assert/v2 v2.10.0 github.com/alecthomas/atomic v0.1.0-alpha2 github.com/alecthomas/chroma/v2 v2.14.0 @@ -68,6 +69,7 @@ require ( golang.org/x/sync v0.8.0 golang.org/x/term v0.23.0 google.golang.org/protobuf v1.34.2 + gotest.tools/v3 v3.5.1 modernc.org/sqlite v1.32.0 ) @@ -87,6 +89,7 @@ require ( github.com/docker/go-units v0.5.0 // indirect github.com/felixge/httpsnoop v1.0.4 // indirect github.com/gogo/protobuf v1.3.2 // indirect + github.com/google/go-cmp v0.6.0 // indirect github.com/gorilla/websocket v1.5.1 // indirect github.com/hashicorp/golang-lru/v2 v2.0.7 // indirect github.com/iancoleman/strcase v0.3.0 // indirect @@ -110,7 +113,6 @@ require ( go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.26.0 // indirect go.opentelemetry.io/proto/otlp v1.3.1 // indirect golang.org/x/tools v0.23.0 // indirect - gotest.tools/v3 v3.5.1 // indirect modernc.org/gc/v3 v3.0.0-20240107210532-573471604cb6 // indirect ) diff --git a/go.sum b/go.sum index 8cfb26cf82..a62e91e5db 100644 --- a/go.sum +++ b/go.sum @@ -16,6 +16,8 @@ github.com/TBD54566975/golang-tools v0.2.1 h1:jzP27dzvJRb43Z9xTbRCPOT/rZD43FZkqV github.com/TBD54566975/golang-tools v0.2.1/go.mod h1:rEEXIq0/pFgZqi/MTOq4DBmVpLHLgI9WocJWXYhu050= github.com/TBD54566975/scaffolder v1.0.0 h1:QUFSy2wVzumLDg7IHcKC6AP+IYyqWe9Wxiu72nZn5qU= github.com/TBD54566975/scaffolder v1.0.0/go.mod h1:auVpczIbOAdIhYDVSruIw41DanxOKB9bSvjf6MEl7Fs= +github.com/XSAM/otelsql v0.32.0 h1:vDRE4nole0iOOlTaC/Bn6ti7VowzgxK39n3Ll1Kt7i0= +github.com/XSAM/otelsql v0.32.0/go.mod h1:Ary0hlyVBbaSwo8atZB8Aoothg9s/LBJj/N/p5qDmLM= github.com/alecthomas/assert/v2 v2.10.0 h1:jjRCHsj6hBJhkmhznrCzoNpbA3zqy0fYiUcYZP/GkPY= github.com/alecthomas/assert/v2 v2.10.0/go.mod h1:Bze95FyfUr7x34QZrjL+XP+0qgp/zg8yS+TtBj1WA3k= github.com/alecthomas/atomic v0.1.0-alpha2 h1:dqwXmax66gXvHhsOS4pGPZKqYOlTkapELkLb3MNdlH8= diff --git a/internal/observability/client_test.go b/internal/observability/client_test.go index 1cb3537ac4..b1582ba137 100644 --- a/internal/observability/client_test.go +++ b/internal/observability/client_test.go @@ -10,5 +10,5 @@ import ( func TestSchemaMismatch(t *testing.T) { dflt := resource.Default() - assert.Equal(t, dflt.SchemaURL(), schemaURL, `change import in client.go to: semconv "go.opentelemetry.io/otel/semconv/v%s"`, path.Base(dflt.SchemaURL())) + assert.Equal(t, dflt.SchemaURL(), schemaURL, `in every file that imports go.opentelemetry.io/otel/semconv, change the import to: semconv "go.opentelemetry.io/otel/semconv/v%s"`, path.Base(dflt.SchemaURL())) } diff --git a/internal/observability/db.go b/internal/observability/db.go new file mode 100644 index 0000000000..f030c43535 --- /dev/null +++ b/internal/observability/db.go @@ -0,0 +1,23 @@ +package observability + +import ( + "database/sql" + "fmt" + + "github.com/XSAM/otelsql" + semconv "go.opentelemetry.io/otel/semconv/v1.4.0" +) + +func OpenDBAndInstrument(dsn string) (*sql.DB, error) { + conn, err := otelsql.Open("pgx", dsn) + if err != nil { + return nil, fmt.Errorf("failed to open database: %w", err) + } + + err = otelsql.RegisterDBStatsMetrics(conn, otelsql.WithAttributes(semconv.DBSystemPostgreSQL)) + if err != nil { + return nil, fmt.Errorf("failed to register database metrics: %w", err) + } + + return conn, nil +} diff --git a/internal/reflect/reflect.go b/internal/reflect/reflect.go index 079c4edb93..2c5aa5e5a9 100644 --- a/internal/reflect/reflect.go +++ b/internal/reflect/reflect.go @@ -13,6 +13,7 @@ package reflect import ( + "container/list" "fmt" "reflect" "strings" @@ -131,6 +132,11 @@ func copyAny(src any, ptrs map[uintptr]any, copyConf *copyConfig) (dst any) { return src } + // Special case list.List to handle its internal structure + if reflect.TypeOf(src) == reflect.TypeFor[*list.List]() { + return copyList(src.(*list.List), ptrs, copyConf) + } + // Look up the corresponding copy function. switch v.Kind() { case reflect.Bool, reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, @@ -146,7 +152,7 @@ func copyAny(src any, ptrs map[uintptr]any, copyConf *copyConfig) (dst any) { dst = copyArray(src, ptrs, copyConf) case reflect.Map: dst = copyMap(src, ptrs, copyConf) - case reflect.Ptr, reflect.UnsafePointer: + case reflect.Pointer, reflect.UnsafePointer: dst = copyPointer(src, ptrs, copyConf) case reflect.Struct: dst = copyStruct(src, ptrs, copyConf) @@ -160,10 +166,22 @@ func copyAny(src any, ptrs map[uintptr]any, copyConf *copyConfig) (dst any) { return } +func copyList(src *list.List, ptrs map[uintptr]any, copyConf *copyConfig) *list.List { + if src == nil { + return nil + } + dst := list.New() + for e := src.Front(); e != nil; e = e.Next() { + copiedValue := copyAny(e.Value, ptrs, copyConf) + dst.PushBack(copiedValue) + } + return dst +} + func copyPremitive(src any, ptr map[uintptr]any, copyConf *copyConfig) (dst any) { kind := reflect.ValueOf(src).Kind() switch kind { - case reflect.Array, reflect.Chan, reflect.Interface, reflect.Map, reflect.Ptr, reflect.Slice, reflect.Struct, reflect.UnsafePointer: + case reflect.Array, reflect.Chan, reflect.Interface, reflect.Map, reflect.Pointer, reflect.Slice, reflect.Struct, reflect.UnsafePointer: panic(fmt.Sprintf("reflect: internal error: type %v is not a primitive", kind)) } dst = src @@ -225,7 +243,7 @@ func copyPointer(x any, ptrs map[uintptr]any, copyConf *copyConfig) any { t := reflect.TypeOf(x) if v.Kind() != reflect.Pointer { - panic(fmt.Errorf("reflect: internal error: must be a Pointer or Ptr; got %v", v.Kind())) + panic(fmt.Errorf("reflect: internal error: must be a Pointer; got %v", v.Kind())) } if v.IsNil() { diff --git a/internal/reflect/reflect_test.go b/internal/reflect/reflect_test.go index a6f138e042..849c9212f7 100644 --- a/internal/reflect/reflect_test.go +++ b/internal/reflect/reflect_test.go @@ -1,9 +1,20 @@ package reflect import ( + "container/list" "testing" + + "gotest.tools/v3/assert" ) +func TestListElements(t *testing.T) { + l := list.New() + l.PushBack("one") + output := DeepCopy(l) + assert.Equal(t, output.Front().Value, l.Front().Value) + assert.Equal(t, output.Len(), l.Len()) +} + type structOfPointers struct { intPtr *int floatPtr *float64