Remove dependency on Jaeger's cmd/agent from jaegerreceiver #38237
Conversation
Signed-off-by: Avinash <avinashchowdhury81@gmail.com>
|
|
||
| jr.settings.Logger.Info("Starting UDP server for Binary Thrift", zap.String("endpoint", jr.config.ThriftBinaryUDP.Endpoint)) | ||
| } | ||
|
|
||
| if jr.config.ThriftCompactUDP != nil { |
There was a problem hiding this comment.
this was conditional before, keep it conditional
|
|
||
| package processor | ||
|
|
||
| type Processor interface { |
There was a problem hiding this comment.
Do you actually need this abstraction?
There was a problem hiding this comment.
It might appear unnecessary. I initially created it to separate the core processing logic from the UDP server implementation
| } | ||
|
|
||
| // Done returns a channel that's closed when the processor has stopped | ||
| func (p *ThriftProcessor) Done() <-chan struct{} { |
There was a problem hiding this comment.
Conceptually Thrift processor is just a handler that you need to pass to UDP Server. I recommend using more traditional setup for receivers
- receiver creates Thrift Handler which is stateless, it only transforms the packet and hands it off to the next trace consumer in the pipeline. Thrift Handler needs no start/stop.
- receiver creates UDP server which has start/stop and runs in a goroutine. The Thrift Handler is passed to server at construction
| "go.uber.org/zap" | ||
| ) | ||
|
|
||
| type PacketHandler interface { |
There was a problem hiding this comment.
Q: is this code mostly copied from Jaeger?
There was a problem hiding this comment.
No, the code is not copied from Jaeger.
There was a problem hiding this comment.
Why not? I'm not going to review brand new UDP code. The existing code is battle tested.
There was a problem hiding this comment.
PR Overview
This PR removes the dependency on Jaeger's cmd/agent by moving the internal agent processing to local packages within the receiver. Key changes include the introduction of a new UDP server implementation, a new Thrift processor that leverages the internal UDP server, and modifications in the trace receiver to use the new processor interface.
Reviewed Changes
| File | Description |
|---|---|
| receiver/jaegerreceiver/internal/udpserver/udp_server.go | Introduces a new UDP server implementation with start/stop functionality and UDP packet handling. |
| receiver/jaegerreceiver/internal/processor/thrift_processor.go | Adds a new Thrift processor using the internal UDP server and removes reliance on external Jaeger dependencies. |
| receiver/jaegerreceiver/internal/processor/processor.go | Defines a new processor interface for consistency across processors. |
| receiver/jaegerreceiver/trace_receiver.go | Updates the receiver to utilize the new processor and removes outdated Jaeger cmd/agent imports. |
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
receiver/jaegerreceiver/internal/udpserver/udp_server.go:1
- The new UDP server implementation is a core component but currently lacks dedicated unit tests. Please add tests to ensure proper error handling and graceful shutdown behavior.
// Copyright The OpenTelemetry Authors
| } | ||
| if opErr, ok := err.(*net.OpError); ok { | ||
| if opErr.Op == "read" && opErr.Net == "udp" { | ||
| if opErr.Err.Error() == "use of closed network connection" { |
There was a problem hiding this comment.
Instead of comparing error strings, consider using errors.Is(err, net.ErrClosed) (or a similar approach) for a more reliable and future-proof check.
| if opErr.Err.Error() == "use of closed network connection" { | |
| if errors.Is(opErr.Err, net.ErrClosed) { |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
Signed-off-by: Avinash <avinashchowdhury81@gmail.com>
Signed-off-by: Avinash <avinashchowdhury81@gmail.com>
Description
This PR moves the jeager's internal
cmd/agentdependenciesLink to tracking issue
Fixes jaegertracing/jaeger#6408,
This PR only removes this dependencies from -
/receiver/jaegerreceiver/trace_receiver.goDependencies are -
Testing
Not Done yet
Documentation