Add gRPC metrics to agent #1180
Conversation
pavolloffay
requested review from
black-adder,
jpkrohling,
objectiser,
tiffon,
vprithvi and
yurishkuro
as code owners
ghost
assigned 
Codecov Report
@@ Coverage Diff @@
## master #1180 +/- ##
======================================
Coverage 100% 100%
======================================
Files 157 159 +2
Lines 7104 7126 +22
======================================
+ Hits 7104 7126 +22
Continue to review full report at Codecov.
|
| reporter: NewReporter(conn, logger), | ||
| manager: NewSamplingManager(conn)}, nil | ||
| reporter: aReporter.WrapWithMetrics(NewReporter(conn, logger), "grpc-reporter", mFactory), | ||
| manager: httpserver.WrapWithMetrics(NewSamplingManager(conn), mFactory)}, nil |
There was a problem hiding this comment.
- not sure why namespace is passed to
reporter.WrapWithMetricsbut not tohttpserver.WrapWithMetrics - also, strictly speaking you can just do
mFactory.Namespace("grpc-reporter", nil)here and not expose WrapWithMetrics to an extra parameter.
There was a problem hiding this comment.
not sure why namespace is passed to reporter.WrapWithMetrics but not to httpserver.WrapWithMetrics
First of all I wanted to keep the old metric names for tchannel reporter. I am wondering whether scoping them based on the propag adds any value. We can make them generic or scope them. I am not sure what is better. The propag itself could also add its specific metrics.
cc @objectiser
also, strictly speaking you can just do mFactory.Namespace("grpc-reporter", nil) here and not expose WrapWithMetrics to an extra parameter.
Can do, passing a string namespace makes it cleaner that it has to be scoped.
There was a problem hiding this comment.
Do you have examples of the metrics for reporter and manager to look at? I assume the reporter will be jaeger_agent_grpc_reporter... similar to jaeger_agent_tc_reporter...?
I think just passing the factory i.e. mFactory.Namespace("grpc-reporter", nil) is better.
There was a problem hiding this comment.
The reporter from master emits this:
jaeger_agent_tchannel_reporter_batch_size{format="jaeger"} 0
jaeger_agent_tchannel_reporter_batch_size{format="zipkin"} 0
jaeger_agent_tchannel_reporter_batches_failures
jaeger_agent_tchannel_reporter_batches_failures{format="jaeger"} 0
jaeger_agent_tchannel_reporter_batches_failures{format="zipkin"} 0
jaeger_agent_tchannel_reporter_batches_submitted
jaeger_agent_tchannel_reporter_batches_submitted{format="jaeger"} 0
jaeger_agent_tchannel_reporter_batches_submitted{format="zipkin"} 0
jaeger_agent_tchannel_reporter_spans_failures
jaeger_agent_tchannel_reporter_spans_failures{format="jaeger"} 0
jaeger_agent_tchannel_reporter_spans_failures{format="zipkin"} 0
jaeger_agent_tchannel_reporter_spans_submitted
jaeger_agent_tchannel_reporter_spans_submitted{format="jaeger"} 0
jaeger_agent_tchannel_reporter_spans_submitted{format="zipkin"} 0
manager:
jaeger_agent_collector_proxy{endpoint="baggage",result="err"} 0
jaeger_agent_collector_proxy{endpoint="baggage",result="ok"} 0
jaeger_agent_collector_proxy{endpoint="sampling",result="err"} 0
jaeger_agent_collector_proxy{endpoint="sampling",result="ok"} 0
Currently the manager does not scope it based on propagation. So do we want to have
jaeger_agent_<propag>_reporter_spans or jaeger_agent_reporter_spans and analogically the same for collector_proxy/manager (might be a subject to renaming)
There was a problem hiding this comment.
I'd vote for making <propag> a label, so jaeger_agent_reporter_spans.
There was a problem hiding this comment.
thanks then this seems like a right way to go
There was a problem hiding this comment.
changed, passing the factory is better 👍
There was a problem hiding this comment.
instead of propag I have used protocol
There was a problem hiding this comment.
Although we use protocol tag to distinguish thrift binary/compat.
jaeger_agent_thrift_udp_server_packet_size{model="jaeger",protocol="binary"} 0
jaeger_agent_thrift_udp_server_packet_size{model="jaeger",protocol="compact"} 0
jaeger_agent_thrift_udp_server_packet_size{model="zipkin",protocol="compact"} 0
jaeger_agent_thrift_udp_t_processor_close_time_bucket{model="jaeger",protocol="binary",le="0.25"}
...
We should find a different tag key. Any suggestions? rpc, propag, source ?..
There was a problem hiding this comment.
Here (UDP server) this refers to inbound protocol. In the Reporter it's outbound, so still works for me.
pavolloffay
force-pushed
the
grpc-metrics-agent
branch
from
ca592da to
d68ca4c
Compare
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
pavolloffay
force-pushed
the
grpc-metrics-agent
branch
from
378ed41 to
8bd42d3
Compare
Signed-off-by: Pavol Loffay <[email protected]>
| jaeger_agent_collector_proxy{endpoint="baggage",protocol="tchannel",result="err"} 0 | ||
| jaeger_agent_collector_proxy{endpoint="baggage",protocol="tchannel",result="ok"} 0 | ||
| jaeger_agent_collector_proxy{endpoint="sampling",protocol="tchannel",result="err"} 0 | ||
| jaeger_agent_collector_proxy{endpoint="sampling",protocol="tchannel",result="ok"} 0 |
There was a problem hiding this comment.
nit: if we remove "EmitZipkinBatch" in #1193, we need to change the changelog since the new metrics won't be partitioned by the "format".
Part of #773
Add gRPC metrics to agent.