feat: add automatic database span coverage to telemetry example#130
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the Goravel example app’s telemetry showcase to include end-to-end coverage for the framework’s new automatic database instrumentation (for facades.DB()), and updates the pinned framework commit accordingly.
Changes:
- Add an automatically-instrumented DB builder query to the
/telemetryendpoint to emit aSELECT usersspan. - Update the telemetry feature test setup to reset ORM state and refresh the database, and assert the new
SELECT usersspan. - Bump the framework replace pin (and
go.sum) to the commit that includes the instrumentation change.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/http/controllers/telemetry_controller.go | Adds a DB builder query intended to emit an automatic SELECT users span under the request trace. |
| tests/feature/telemetry_test.go | Resets ORM + refreshes DB in suite setup and asserts the additional DB span in Jaeger. |
| go.mod | Updates the replace pin for github.com/goravel/framework to the commit containing the instrumentation work. |
| go.sum | Aligns module checksums with the updated framework pin. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dd3e776 to
a21fed6
Compare
a990421 to
dd8d94c
Compare
f446534 to
c31f156
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
app/http/controllers/telemetry_controller.go:58
- PR description says the gRPC config key was restored from
grpc.clientsback togrpc.servers, butconfig/grpc.goin this branch still definesgrpc.clients. If the framework client readsgrpc.servers.<name>.host(as described),facades.Grpc().Connect("user")will still fail and/telemetrywill return 500 before emitting the expected gRPC spans.
resp, err := facades.Http().WithContext(ctx).Get("/grpc/user?token=1")
if err != nil {
return ctx.Response().Json(http.StatusInternalServerError, http.Json{
"error": err.Error(),
})
}
c31f156 to
04e0a70
Compare
hwbrzzl
left a comment
There was a problem hiding this comment.
LGTM, could you fix these two failures? And I had fixed the failed CI in master.
--- FAIL: TestTelemetryTestSuite (95.71s)
--- FAIL: TestTelemetryTestSuite/TestTraces (60.08s)
telemetry_test.go:54: telemetry not found at http://localhost:16686/api/traces?service=goravel-plain
want: ["GET /telemetry" "HTTP GET" "user.UserService/GetUser" "GET /grpc/user" "users.process" "users.consume" "SELECT users"]
--- FAIL: TestTelemetryTLSTestSuite (72.78s)
--- FAIL: TestTelemetryTLSTestSuite/TestTraces (60.07s)
telemetry_tls_test.go:69: telemetry not found at http://localhost:16686/api/traces?service=goravel-tls
want: ["GET /telemetry" "user.UserService/GetUser"]
The telemetry example exercised HTTP, gRPC, manual spans, and logs but not database queries. With framework #1477 (database auto instrumentation) merged, facades.DB() emits spans automatically, so run an instrumented builder query in the /telemetry handler and assert the resulting "SELECT users" span end to end. The DB connection and its bound tracer are cached process wide, so the suite clears the connection cache (Orm().Fresh) and drops the resolved DB facade (App().Fresh(binding.DB)) after overriding the service name, which keeps the span under the suite's own service even when an earlier suite already built the connection. Bumps the framework pin to the commit containing #1477.
The gRPC client config key was renamed to "clients", but the framework reads grpc.servers.<name>.host in Connect(), so the user client host resolved empty and every gRPC request failed with "client's host can't be empty". Restore the "servers" key. This unblocks the gRPC and telemetry feature suites on ubuntu CI.
Framework master (pinned via the merge) renamed the gRPC client config key back to grpc.clients in #1506, so use "clients" to match what Connect() now reads. Also clarify the telemetry suite reset: the DB connection and its bound tracer are cached process-wide and survive App().Restart(), so Orm().Fresh() plus App().Fresh(binding.DB) are required to rebind the "SELECT users" span under the suite's own service name. Verified by reproducing the CI ordering (a DB suite before the telemetry suite, -p 1): the span is missing without these lines.
Framework PR #1509 resets database connections when the application rebuilds, so app.Restart() (called by tests.OverrideConfig) now rebinds the DB connection under the suite's service name on its own. Remove the manual Orm().Fresh() and App().Fresh(binding.DB) calls from the telemetry suite and pin the framework commit that contains the fix.
The /telemetry handler now runs a DB query, so the TLS suite must also migrate the users table for the request to succeed, matching the plain telemetry suite.
2c80fbf to
c7bb44d
Compare
hwbrzzl
left a comment
There was a problem hiding this comment.
Tested locally, awesome!
📑 Description
RelatedTo goravel/goravel#726
Framework #1477 added automatic database instrumentation for both
facades.Orm()andfacades.DB(). The framework module has no SQLite driver, so a live database span can only be asserted end to end here, against a real database and the collector stack.What is changing:
/telemetryhandler now runs an automatically instrumented builder query (facades.DB().WithContext(ctx).Table("users").Limit(1).Get(...)). No manual tracing: the query emits aSELECT usersclient span nested under the request span, the same wayfacades.Orm()does.TestTracesasserts theSELECT usersspan alongside the existing HTTP, gRPC, manual, and consume spans.Example:
Depends on goravel/framework#1509:
The DB connection (and the tracer bound to it) is cached process-wide. The telemetry suite swaps
telemetry.service.nameviaOverrideConfig, which restarts the app, so the connection must be rebuilt under the new service for the span to report correctly. goravel/framework#1509 makesapp.Restart()reset database connections, so this suite needs no manualOrm().Fresh()/App().Fresh(binding.DB)workaround. CI here stays red until that framework PR merges into master.✅ Checks