-
Notifications
You must be signed in to change notification settings - Fork 108
feat: [#726] Add HTTP server and client telemetry instrumentation [5] #1326
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds HTTP server and client telemetry instrumentation capabilities to the Goravel framework, enabling automatic tracing and metrics collection for HTTP requests and responses. This is part of the broader telemetry instrumentation feature set (issue #726).
Key Changes:
- Implemented HTTP middleware for server-side request instrumentation with configurable filtering
- Created HTTP client transport wrapper for outbound request instrumentation
- Added configuration support for HTTP server instrumentation with exclusion patterns
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
telemetry/setup/stubs.go |
Adds instrumentation configuration section with http_server settings for enabled state and path/method exclusions |
telemetry/instrumentation/http/middleware.go |
Implements Telemetry middleware that captures traces, metrics, and handles panics for incoming HTTP requests |
telemetry/instrumentation/http/middleware_test.go |
Provides test suite with test helpers for middleware functionality including success cases, exclusions, disabled state, and panic handling |
telemetry/instrumentation/http/config.go |
Defines ServerConfig structure and option functions for customizing HTTP server instrumentation behavior |
telemetry/instrumentation/http/transport.go |
Implements NewTransport function to wrap HTTP clients with telemetry instrumentation |
telemetry/instrumentation/http/transport_test.go |
Tests for transport wrapper covering fallback scenarios and successful wrapping cases |
go.mod |
Adds dependency on otelhttp instrumentation library |
go.sum |
Updates dependency checksums for otelhttp and its transitive dependency httpsnoop |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1326 +/- ##
==========================================
- Coverage 70.22% 70.15% -0.08%
==========================================
Files 281 284 +3
Lines 16852 16995 +143
==========================================
+ Hits 11834 11922 +88
- Misses 4525 4568 +43
- Partials 493 505 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@krishankumar01 No, just using the default configuration and run the project. |
|
Okay, I’ll check. I’m thinking of using a kill switch for all instrumentation in Instrumentation:
HttpServer:
Enabled: true
HttpClient:
Enabled: false
Log:
Enabled: falseIn this case, regardless of whether the log driver or HTTP middleware is registered, telemetry will not be published when it is disabled. |
@hwbrzzl I think the issue is in |
foundation/application_builder.go
Outdated
| } | ||
|
|
||
| // Register routes | ||
| for _, route := range r.routes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving the route registration to the end, because if a user registers any gRPC stats handler or interceptors after calling routes.Grpc() (since in this file we usually call facades.Grpc().Server()), they will be ignored as the server is initialized only once in grpc/application.go.
log/application.go
Outdated
| } | ||
| case log.DriverOtel: | ||
| logLogger := telemetrylog.NewTelemetryChannel() | ||
| logLogger := telemetrylog.NewTelemetryChannel(config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won’t work if we call the otel driver logger inside providers or from the application builder callbacks, or any callback that runs before provider bootstrap, because telemetry.TelemetryFacade won’t be initialized and therefore won’t be able to lazily initialize the logger. This is expected, as telemetry is typically available only within the request lifecycle context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about passing TelemetryFacade here instead of using telemetry.TelemetryFacade directly? telemetry/instrumentation/* should not use the global telemetry.TelemetryFacade and telemetry.ConfigFacade. They should be passed into.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then how do we ensure that a telemetry log channel can accept a telemetry facade instance? When we initialize the telemetry channel in NewApplication, the telemetry facade won’t be available yet. This would require making telemetry a dependency of the log facade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we initialize the telemetry channel in NewApplication, the telemetry facade won’t be available yet.
It should be impossible if the telemetry facade is registered, the telemetry channel should not be initialized during registering service providers but during booting. Return an error here if the telemetry facade is nil during booting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if the user sets the OTEL driver as the default or includes it in the stack, then log.NewApplication will try to resolve the telemetry channel instance, which depends on the telemetry facade. Since telemetry depends on log facade it will register and boot after it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically,
facades.*should be used inside a callback function if users want to use them inbootstrap.go.
Can we also add an annotation here explaining the different scenarios in which this(WithCallback) should be used? Since this is called before bootstrapping the service providers, it would be helpful to clarify that. Could you please add that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The optimization is a bit complex, I need about two or three days. Please continue other parts for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, In mean time I will work on #1339.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done


📑 Description
RelatedTo goravel/goravel#726
✅ Checks