Add middleware option to supply StartSpanOptions#80
Conversation
|
@lucacome drawing your attention to this since you made most of the recent contributions. Does this change make sense? |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a middleware option to supply custom StartSpanOptions for tracing, enabling users to append additional tags (e.g. via opentracing.Tag) to spans for dynamic sampling decisions.
- Introduces the MWStartSpanOptions middleware option and its corresponding helper function.
- Adds a new test (TestStartSpanOptionsOption) to verify that the custom StartSpanOptions are correctly applied.
- Updates the MiddlewareFunc and supporting collectStartSpanOptions function in server.go to integrate the new option.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| nethttp/server_test.go | New test case added to validate the behavior of the StartSpanOptions. |
| nethttp/server.go | New MWStartSpanOptions option added and integrated into the middleware. |
Comments suppressed due to low confidence (2)
nethttp/server_test.go:250
- [nitpick] Using the string literal "" to represent a missing tag in the test may lead to confusion. Consider using an empty string or a different sentinel value to clearly indicate the absence of the tag.
if !ok {
nethttp/server.go:183
- [nitpick] The default RPCServerOption is prepended before user-supplied options, which may limit users' ability to override its behavior. Consider documenting the ordering or allowing user options to take precedence if intended.
startSpanOptions = append(startSpanOptions, mwStartSpanOptions...)
|
@lucacome any comments? This one from copilot seem like a false-positive:
The test already uses |
|
@lucacome in case you do not have time, maybe there's someone else who could do the review of this PR? |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request adds a new ChangesSpan options configuration
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nethttp/server.go`:
- Around line 55-58: The MWStartSpanOptions setter should guard against a nil
callback: when MWStartSpanOptions(f) is called with f == nil, normalize to a
no-op function instead of storing nil on mwOptions.startSpanOptions so
downstream code (the request handling path that calls options.startSpanOptions
or the code at lines ~179-180) cannot panic; update the MWStartSpanOptions
implementation to assign a default func(r *http.Request)
[]opentracing.StartSpanOption { return nil } whenever f is nil, or add a
nil-check and conditional invocation at the site that calls
options.startSpanOptions to ensure it is not dereferenced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe6689fc-17c6-4a9d-b8f2-f7b435513efd
📒 Files selected for processing (2)
nethttp/server.gonethttp/server_test.go
| func MWStartSpanOptions(f func(r *http.Request) []opentracing.StartSpanOption) MWOption { | ||
| return func(options *mwOptions) { | ||
| options.startSpanOptions = f | ||
| } |
There was a problem hiding this comment.
Guard against nil MWStartSpanOptions callback to prevent request-path panic.
If a caller passes MWStartSpanOptions(nil), Line 180 dereferences a nil function and panics for every request. Please normalize nil to a no-op callback (and/or guard before invocation).
Proposed fix
func MWStartSpanOptions(f func(r *http.Request) []opentracing.StartSpanOption) MWOption {
return func(options *mwOptions) {
- options.startSpanOptions = f
+ if f == nil {
+ options.startSpanOptions = func(r *http.Request) []opentracing.StartSpanOption { return nil }
+ return
+ }
+ options.startSpanOptions = f
}
}
@@
func collectStartSpanOptions(ctx opentracing.SpanContext, r *http.Request, opts mwOptions) []opentracing.StartSpanOption {
- mwStartSpanOptions := opts.startSpanOptions(r)
+ if opts.startSpanOptions == nil {
+ return []opentracing.StartSpanOption{ext.RPCServerOption(ctx)}
+ }
+ mwStartSpanOptions := opts.startSpanOptions(r)
startSpanOptions := make([]opentracing.StartSpanOption, 0, len(mwStartSpanOptions)+1)
startSpanOptions = append(startSpanOptions, ext.RPCServerOption(ctx))
startSpanOptions = append(startSpanOptions, mwStartSpanOptions...)Also applies to: 179-180
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nethttp/server.go` around lines 55 - 58, The MWStartSpanOptions setter should
guard against a nil callback: when MWStartSpanOptions(f) is called with f ==
nil, normalize to a no-op function instead of storing nil on
mwOptions.startSpanOptions so downstream code (the request handling path that
calls options.startSpanOptions or the code at lines ~179-180) cannot panic;
update the MWStartSpanOptions implementation to assign a default func(r
*http.Request) []opentracing.StartSpanOption { return nil } whenever f is nil,
or add a nil-check and conditional invocation at the site that calls
options.startSpanOptions to ensure it is not dereferenced.
This commit adds a middleware option that supplies
StartSpanOptionsforStartSpan()call.Concrete use case where this is useful: adding additional tags to the span through
opentracing.Tag{}option, so that those tags could be used for deciding whether to sample this span.Summary by CodeRabbit