Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: register db.sql metrics #2305

Merged
merged 10 commits into from
Aug 13, 2024
Merged

feat: register db.sql metrics #2305

merged 10 commits into from
Aug 13, 2024

Conversation

deniseli
Copy link
Contributor

@deniseli deniseli commented Aug 8, 2024

Fixes #2285

Register the standard otel database metrics using: https://github.com/XSAM/otelsql
Found call sites by grepping for sql.Open and skipping all test code.

Sample:

ScopeMetrics #2
ScopeMetrics SchemaURL: 
InstrumentationScope github.com/XSAM/otelsql 0.32.0

Metric #0
Descriptor:
     -> Name: db.sql.latency
     -> Description: The latency of calls in milliseconds
     -> Unit: ms
     -> DataType: Histogram
     -> AggregationTemporality: Cumulative

HistogramDataPoints #0
Data point attributes:
     -> method: Str(sql.conn.begin_tx)
     -> status: Str(ok)
StartTimestamp: 2024-08-08 23:18:01.011762 +0000 UTC
Timestamp: 2024-08-08 23:18:11.012177 +0000 UTC
Count: 23
Sum: 17.082795
Min: 0.137584
Max: 3.233917

@ftl-robot ftl-robot mentioned this pull request Aug 8, 2024
@deniseli deniseli marked this pull request as ready for review August 8, 2024 23:27
@deniseli deniseli requested a review from alecthomas as a code owner August 8, 2024 23:27
@deniseli deniseli requested review from a team and worstell and removed request for a team August 8, 2024 23:27
Copy link
Collaborator

@alecthomas alecthomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with some comments that need to be addressed.

@@ -51,7 +52,9 @@ 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 := otelsql.Open("pgx", cli.ControllerConfig.DSN)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a nice clean API.

cmd/ftl/cmd_box_run.go Show resolved Hide resolved
cmd/ftl/cmd_serve.go Show resolved Hide resolved
internal/modulecontext/database.go Show resolved Hide resolved
@deniseli
Copy link
Contributor Author

deniseli commented Aug 9, 2024

Looks like this is breaking tests right now because modulecontext's Build() calls internal/reflect's DeepCopy, which doesn't expect any values of kind=string that can't be cast to a string. The databases now include a bunch of metric.descOpt{...}s wrapping the string descriptions of each metric, so that's confusing the cloning. I'll pick this back up in the morning

@deniseli
Copy link
Contributor Author

@deniseli
Copy link
Contributor Author

New changes: 2 minor additions to internal/reflect/reflect.go:

  • Added support for string aliases. Before, this would cast the src string as long as its Kind was string, but that would cause a panic when the true type was an alias. In this case, it was metric.descOpt.
  • Added support for the list.List type, which is used by the global.meter type inside otelsql.otConnector. Before, DeepCopy would panic whenever a list.List was given as input with reflect.Set: value of type *list.List is not assignable to type *list.Element. This adds a special case for that type and performs a standard linked-list deep copy, which is easier than managing consistency across all the backpointers and parent pointers in that structure.

ftl-project.toml Outdated
@@ -1,8 +1,6 @@
name = "ftl"
module-dirs = ["examples/go"]
ftl-min-version = ""
hermit = false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why'd these get removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh what the heck! I'll undo all the changes to that file. Thanks for catching that

@deniseli deniseli added this pull request to the merge queue Aug 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 13, 2024
@deniseli deniseli enabled auto-merge August 13, 2024 21:49
@deniseli deniseli added this pull request to the merge queue Aug 13, 2024
Merged via the queue into main with commit 1028e65 Aug 13, 2024
17 checks passed
@deniseli deniseli deleted the dli/otelsql branch August 13, 2024 21:55
safeer added a commit that referenced this pull request Aug 14, 2024
github-merge-queue bot pushed a commit that referenced this pull request Aug 14, 2024
@deniseli deniseli restored the dli/otelsql branch August 14, 2024 19:58
github-merge-queue bot pushed a commit that referenced this pull request Aug 20, 2024
Fixes #2285

Unreverts #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`
* #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] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

otel: instrument observability (metrics + traces) on the database
2 participants