feat: extensible OTEL span filtering and naming#81
Closed
thoriqzs25 wants to merge 2 commits intogo-coldbrew:mainfrom
Closed
feat: extensible OTEL span filtering and naming#81thoriqzs25 wants to merge 2 commits intogo-coldbrew:mainfrom
thoriqzs25 wants to merge 2 commits intogo-coldbrew:mainfrom
Conversation
Add configurable span filtering and transformation for OpenTelemetry traces: - SpanFilter/SpanTransformer interfaces for client extensibility - OTEL_GRPC_SPAN_NAME_FORMAT=short|full for gRPC span naming - OTEL_FILTER_SPAN_NAMES for exact-match span filtering - AddOTELSpanFilter() and AddOTELSpanTransformer() runtime APIs
📝 WalkthroughWalkthroughAdds OpenTelemetry span filtering and transformation: new config fields for gRPC span-name format and filtered span names, a configurable SpanProcessor that filters/transforms spans before batching/export, runtime APIs to register filters/transformers, and tests covering behavior and concurrency. Changes
Sequence Diagram(s)sequenceDiagram
participant Tracer as Tracer/SDK
participant SpanProc as Custom SpanProcessor
participant Batch as OTLP Batch Processor
participant Exporter as OTLP Exporter
Tracer->>SpanProc: OnStart / OnEnd(span)
SpanProc->>SpanProc: apply FilterSpanNames / custom Filters
alt span not dropped
SpanProc->>SpanProc: optionally rename (gRPC short / transformers)
SpanProc->>Batch: forward span
Batch->>Exporter: batch & export
else span dropped
SpanProc-->>Tracer: drop (no forward)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 3
🧹 Nitpick comments (1)
initializers.go (1)
170-171: Synchronize access to the global span processor.
SetupOpenTelemetryreplacesotelSpanProcessorwhile the runtime registration APIs read it without any synchronization. That gives you a-racehit if filters/transformers are added during startup or after a re-init; anatomic.Pointeror small mutex around load/store would make the runtime API actually thread-safe.Also applies to: 542-555
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@initializers.go` around lines 170 - 171, The global variable otelSpanProcessor is accessed concurrently by SetupOpenTelemetry (which replaces it) and runtime registration APIs that read it, causing race conditions; synchronize access by changing otelSpanProcessor to an atomic.Pointer[*cbotel.SpanProcessor] or guard all loads/stores with a small sync.Mutex. Update SetupOpenTelemetry to store the new processor via atomic.StorePointer (or mutex-protected assignment) and update all runtime readers (the registration paths that load otelSpanProcessor / functions that add filters/transformers) to use atomic.LoadPointer (or lock the mutex) before dereferencing; ensure any nil checks follow the same atomic/mutex pattern so reads and writes are consistently synchronized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/config.go`:
- Around line 183-189: Add validation for the OTELGRPCSpanNameFormat config
field inside the existing Validate() function so values other than "short" or
"full" produce a warning and the code falls back to "full"; specifically, check
the OTELGRPCSpanNameFormat string (from the struct field named
OTELGRPCSpanNameFormat) and if it is non-empty and not equal to "short" or
"full", emit a warning via the same logger used by other Validate() checks and
set/keep the value as "full" to preserve current behavior.
In `@otel/spanprocessor.go`:
- Around line 160-171: Modify SpanProcessor.AddFilter and AddTransformer to
reject nil registrations up-front: check the incoming SpanFilter (and adapters
like SpanFilterFunc(nil)) and SpanTransformer (and SpanTransformerFunc(nil)) for
nil before locking or appending, and either return early (no-op) or return an
error/boolean to the caller; ensure you update the methods that call these (and
document behavior) so a nil filter/transformer is not added to p.config.Filters
or p.config.Transformers and thus cannot cause a panic in OnEnd. Use the
existing method names SpanProcessor.AddFilter, SpanProcessor.AddTransformer and
reference p.config.Filters / p.config.Transformers and OnEnd when making the
change.
- Around line 27-31: The doc comment for SpanTransformer is misleading because
the Transform method only returns a replacement name and cannot mutate span
attributes; update the comments to state this is a renaming-only contract:
change the top comment to say SpanTransformer allows clients to rename spans
before export (not add attributes or mutate spans), and update the Transform
method comment to explicitly state it returns a new span name (return empty
string to keep the original) and that no other span data is modified; reference
SpanTransformer and Transform (taking sdktrace.ReadOnlySpan) so readers know
which symbols are constrained.
---
Nitpick comments:
In `@initializers.go`:
- Around line 170-171: The global variable otelSpanProcessor is accessed
concurrently by SetupOpenTelemetry (which replaces it) and runtime registration
APIs that read it, causing race conditions; synchronize access by changing
otelSpanProcessor to an atomic.Pointer[*cbotel.SpanProcessor] or guard all
loads/stores with a small sync.Mutex. Update SetupOpenTelemetry to store the new
processor via atomic.StorePointer (or mutex-protected assignment) and update all
runtime readers (the registration paths that load otelSpanProcessor / functions
that add filters/transformers) to use atomic.LoadPointer (or lock the mutex)
before dereferencing; ensure any nil checks follow the same atomic/mutex pattern
so reads and writes are consistently synchronized.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 042587bb-c128-4453-a400-83526e4a565c
📒 Files selected for processing (5)
config/config.gocore.goinitializers.gootel/spanprocessor.gootel/spanprocessor_test.go
Comment on lines
+183
to
+189
| // OTELGRPCSpanNameFormat controls gRPC span naming. | ||
| // "short" extracts just the method name (e.g., "V0GetStats") | ||
| // "full" keeps the full path (e.g., "/pkg.Service/V0GetStats") - default | ||
| OTELGRPCSpanNameFormat string `envconfig:"OTEL_GRPC_SPAN_NAME_FORMAT" default:"full"` | ||
| // OTELFilterSpanNames is a comma-separated list of span names to filter out (exact match). | ||
| // Common use: "ServeHTTP" to filter HTTP transport spans. | ||
| OTELFilterSpanNames string `envconfig:"OTEL_FILTER_SPAN_NAMES" default:""` |
There was a problem hiding this comment.
Warn on unsupported OTEL_GRPC_SPAN_NAME_FORMAT values.
Anything other than "short" currently falls back to full naming silently, so a typo just disables the feature. Adding a Validate() warning here would match how other enum-like config fields are handled in this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/config.go` around lines 183 - 189, Add validation for the
OTELGRPCSpanNameFormat config field inside the existing Validate() function so
values other than "short" or "full" produce a warning and the code falls back to
"full"; specifically, check the OTELGRPCSpanNameFormat string (from the struct
field named OTELGRPCSpanNameFormat) and if it is non-empty and not equal to
"short" or "full", emit a warning via the same logger used by other Validate()
checks and set/keep the value as "full" to preserve current behavior.
- Add config validation for OTELGRPCSpanNameFormat (warn if not short/full) - Clarify SpanTransformer docs: renaming-only contract - Nil-safe AddFilter/AddTransformer to prevent OnEnd panics - Add tests for validation and nil handling
There was a problem hiding this comment.
♻️ Duplicate comments (1)
otel/spanprocessor.go (1)
73-82:⚠️ Potential issue | 🔴 CriticalTyped-nil adapters can still panic at runtime.
Line 162 and Line 173 only reject interface-nil.
SpanFilterFunc(nil)/SpanTransformerFunc(nil)pass these checks, get stored, and can panic when invoked inOnEnd/maybeTransform. The same risk exists for initialconfig.Filters/config.Transformerspassed at construction.💡 Proposed fix
type SpanProcessor struct { next sdktrace.SpanProcessor config SpanProcessorConfig filterNames map[string]struct{} mu sync.RWMutex } +func isNilSpanFilter(f SpanFilter) bool { + if f == nil { + return true + } + if fn, ok := f.(SpanFilterFunc); ok && fn == nil { + return true + } + return false +} + +func isNilSpanTransformer(t SpanTransformer) bool { + if t == nil { + return true + } + if fn, ok := t.(SpanTransformerFunc); ok && fn == nil { + return true + } + return false +} + // NewSpanProcessor creates a new SpanProcessor wrapping the given processor. func NewSpanProcessor(next sdktrace.SpanProcessor, config SpanProcessorConfig) *SpanProcessor { filterNames := make(map[string]struct{}, len(config.FilterSpanNames)) for _, name := range config.FilterSpanNames { filterNames[name] = struct{}{} } + filters := make([]SpanFilter, 0, len(config.Filters)) + for _, f := range config.Filters { + if !isNilSpanFilter(f) { + filters = append(filters, f) + } + } + transformers := make([]SpanTransformer, 0, len(config.Transformers)) + for _, t := range config.Transformers { + if !isNilSpanTransformer(t) { + transformers = append(transformers, t) + } + } + config.Filters = filters + config.Transformers = transformers return &SpanProcessor{ next: next, config: config, filterNames: filterNames, } } @@ func (p *SpanProcessor) AddFilter(f SpanFilter) { - if f == nil { + if isNilSpanFilter(f) { return } p.mu.Lock() defer p.mu.Unlock() p.config.Filters = append(p.config.Filters, f) } @@ func (p *SpanProcessor) AddTransformer(t SpanTransformer) { - if t == nil { + if isNilSpanTransformer(t) { return } p.mu.Lock() defer p.mu.Unlock() p.config.Transformers = append(p.config.Transformers, t) }#!/bin/bash # Verify whether typed-nil adapters are currently guarded and whether tests cover them. set -euo pipefail echo "== Guards in registration paths ==" rg -n -C3 'func \(p \*SpanProcessor\) AddFilter|func \(p \*SpanProcessor\) AddTransformer|if f == nil|if t == nil' otel/spanprocessor.go echo echo "== Adapter definitions ==" rg -n -C2 'type SpanFilterFunc|type SpanTransformerFunc' otel/spanprocessor.go echo echo "== Typed-nil test coverage ==" rg -n -C2 'SpanFilterFunc\(nil\)|SpanTransformerFunc\(nil\)|typed nil|nil filter|nil transformer' --type goAlso applies to: 161-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otel/spanprocessor.go` around lines 73 - 82, NewSpanProcessor, AddFilter, and AddTransformer currently accept SpanFilterFunc/SpanTransformerFunc values that can be typed-nil and later panic when invoked in OnEnd/maybeTransform; update all registration paths (NewSpanProcessor iterating config.Filters/config.Transformers, and methods AddFilter/AddTransformer) to reject typed-nil adapters by checking both interface-nil and typed-nil (use reflect.ValueOf(x).IsValid()/IsNil() for the underlying value) and skip or return an error for such nil functions, and also add a defensive nil check before invoking stored adapters in maybeTransform/OnEnd to avoid panics referencing SpanFilterFunc, SpanTransformerFunc, maybeTransform, OnEnd, config.Filters and config.Transformers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@otel/spanprocessor.go`:
- Around line 73-82: NewSpanProcessor, AddFilter, and AddTransformer currently
accept SpanFilterFunc/SpanTransformerFunc values that can be typed-nil and later
panic when invoked in OnEnd/maybeTransform; update all registration paths
(NewSpanProcessor iterating config.Filters/config.Transformers, and methods
AddFilter/AddTransformer) to reject typed-nil adapters by checking both
interface-nil and typed-nil (use reflect.ValueOf(x).IsValid()/IsNil() for the
underlying value) and skip or return an error for such nil functions, and also
add a defensive nil check before invoking stored adapters in
maybeTransform/OnEnd to avoid panics referencing SpanFilterFunc,
SpanTransformerFunc, maybeTransform, OnEnd, config.Filters and
config.Transformers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c239ecee-b2b5-4ae1-87cf-2ea29b6c0be6
📒 Files selected for processing (4)
config/config.goconfig/config_test.gootel/spanprocessor.gootel/spanprocessor_test.go
✅ Files skipped from review due to trivial changes (1)
- otel/spanprocessor_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- config/config.go
Author
|
Raised issue #80 |
Member
|
Thanks for this @thoriqzs25, a lot of these changes have been pushed in the last few release, please update coldbrew and retry |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add configurable span filtering and transformation for OpenTelemetry traces:
Description
Adds extensible OTEL span customization to reduce trace noise and improve span readability.
Package(s) Affected
Checklist
make test)make lint)make doc)Summary by CodeRabbit
New Features
Improvements