feat(grpc): adds support for grpc parsing. #177
Conversation
jcchavezs
force-pushed
the
grpc_parsers
branch
from
aa10f0e
to
41a7b81
Compare
jcchavezs
force-pushed
the
grpc_parsers
branch
from
41a7b81
to
1c762e0
Compare
jcchavezs
force-pushed
the
grpc_parsers
branch
from
a5b3b7b
to
9856169
Compare
jcchavezs
force-pushed
the
grpc_parsers
branch
from
9856169
to
92e7337
Compare
| @@ -27,6 +27,24 @@ import ( | |||
| "github.com/openzipkin/zipkin-go/model" | |||
| ) | |||
|
|
|||
| type handleRPCParser struct { | |||
| inPayload func(*stats.InPayload, zipkin.SpanCustomizer) | |||
There was a problem hiding this comment.
This functions might become func(*stats.InPayload, model.SpanContext, zipkin.SpanCustomizer) because SpanCustomizer does not expose access to the context.
There was a problem hiding this comment.
I would expect RPC parser to be slightly more abstract.. It is a bit easier though possibly easier to see this later, when there's a request and response type instead of matching exact grpc lifecycle. I mean it would be easier to see if another RPC model was in use. ex https://github.com/openzipkin/brave/blob/master/instrumentation/grpc/src/main/java/brave/grpc/GrpcRequest.java
| @@ -39,3 +39,5 @@ func (*noopSpan) Finish() {} | |||
| func (*noopSpan) FinishedWithDuration(duration time.Duration) {} | |||
|
|
|||
| func (*noopSpan) Flush() {} | |||
|
|
|||
| func (*noopSpan) IsNoop() bool { return true } | |||
There was a problem hiding this comment.
IsNoop is a convenient function to verify whether a span is noop or not and don't run expensive parsing when the span is noop.
|
|
||
| // SpanCustomizer allows to safely customize a span without accesing its lifecycle | ||
| // methods | ||
| type SpanCustomizer interface { |
There was a problem hiding this comment.
name is notably missing here and needed I think https://github.com/openzipkin/brave/tree/master/instrumentation/grpc#rpc-model-mapping
There was a problem hiding this comment.
note: in brave we used to have more direct mapping to grpc hooks, but later changed that to match how we parse other things (deprecated the approach that looks more like this). It might be worth having a look as one outcome that could be nice from this is being able to adapt data similarly.
| @@ -27,6 +27,24 @@ import ( | |||
| "github.com/openzipkin/zipkin-go/model" | |||
| ) | |||
|
|
|||
| type handleRPCParser struct { | |||
| inPayload func(*stats.InPayload, zipkin.SpanCustomizer) | |||
There was a problem hiding this comment.
I would expect RPC parser to be slightly more abstract.. It is a bit easier though possibly easier to see this later, when there's a request and response type instead of matching exact grpc lifecycle. I mean it would be easier to see if another RPC model was in use. ex https://github.com/openzipkin/brave/blob/master/instrumentation/grpc/src/main/java/brave/grpc/GrpcRequest.java
|
|
||
| // SpanCustomizer allows to safely customize a span without accesing its lifecycle | ||
| // methods | ||
| type SpanCustomizer interface { |
There was a problem hiding this comment.
name is notably missing here and needed I think https://github.com/openzipkin/brave/tree/master/instrumentation/grpc#rpc-model-mapping
This is a very early start of a change that does two things:
Depends on #181
Ping @basvanbeek @anuraaga @adriancole @fho