-
Notifications
You must be signed in to change notification settings - Fork 328
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.
Thanks!
Left two high-level comments before reviewing further.
exporter/jaeger/jaeger.go
Outdated
@@ -39,6 +39,10 @@ type Options struct { | |||
// For example, http://localhost:14268. | |||
Endpoint string | |||
|
|||
// LocalAgentHostPort instructs exporter to send spans to jaeger-agent at this address. | |||
// For example, localhost:6831. | |||
LocalAgentHostPort string |
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.
Can we call this AgentEndpoint? As far as, I can understand, agent can work on UDP and TCP. Don't we need to give options to the user?
Maybe it might make sense to have a net.Conn field.
// ...
AgentConn *net.Conn
Then, user can dial it themselves:
conn, err := net.Dial("tcp", "endpoint:port")
and we can type cast conn to see whether we should use the UDP or TCP endpoint.
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.
@rakyll Updated, As far as I know, agent only works on UDP. http://jaeger.readthedocs.io/en/latest/deployment/
examples/trace/jaeger/agent/main.go
Outdated
"go.opencensus.io/trace" | ||
) | ||
|
||
func main() { |
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.
I think a full example is too much for the agent support, but we can add a godoc example demonstrating how to create an exporter that uses the agent.
84fc0a1
to
4d6ee27
Compare
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.
@rakyll are you ok to merge this? |
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.
Left some comments.
exporter/jaeger/agent.go
Outdated
) | ||
|
||
// UDPPacketMaxLength is the max size of UDP packet we want to send, synced with jaeger-agent | ||
const UDPPacketMaxLength = 65000 |
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.
Don't export.
udpPacketMaxLength.
exporter/jaeger/agent.go
Outdated
const UDPPacketMaxLength = 65000 | ||
|
||
// AgentClientUDP is a UDP client to Jaeger agent that implements gen.Agent interface. | ||
type AgentClientUDP struct { |
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.
Don't export this, these types should not be visible to the user.
agentClientUDP
exporter/jaeger/agent.go
Outdated
} | ||
|
||
// NewAgentClientUDP creates a client that sends spans to Jaeger Agent over UDP. | ||
func NewAgentClientUDP(hostPort string, maxPacketSize int) (*AgentClientUDP, error) { |
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.
ditto, newAgentClientUDP
// Register the Jaeger exporter to be able to retrieve | ||
// the collected spans. | ||
exporter, err := jaeger.NewExporter(jaeger.Options{ | ||
AgentEndpoint: "localhost:6831", |
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.
Agent can listen on TCP, right? Are we doing anything to allow user to depend on an TCP endpoint.
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.
@rakyll, As far as I know, agent only works on UDP. http://jaeger.readthedocs.io/en/latest/deployment/
d6e0b0b
to
054e41c
Compare
Thanks! |
Build is broken due to a bad merge, census-instrumentation#518.
Build is broken due to a bad merge, #518.
Almost users use jaeger agent to report trace data.