Add client and server tracing interceptors #1756
Conversation
|
|
||
| class TracingTestCase: XCTestCase { | ||
| override class func setUp() { | ||
| InstrumentationSystem.bootstrap(TestTracer()) |
There was a problem hiding this comment.
Does this blow up like logging/metrics?
There was a problem hiding this comment.
You mean if it fails when called more than once? If so, yes, and that's why I had to do it here, so that it's called just once.
| tracer.inject( | ||
| serviceContext, | ||
| into: &request, | ||
| using: ClientRequestInjector() |
There was a problem hiding this comment.
Two questions here:
- Should we create the
ClientRequestInjectoronly once in the init of this struct? - Is there a case where the user might want to provide their own injector here?
There was a problem hiding this comment.
- 👍 - had to change the
Carriertype to beMetadatato avoid generics hell. - I don't think so, really - these injectors are provided as part of the basic interceptor we present here. If users need more control I think they could just create their own interceptors.
| return try await tracer.withSpan(context.descriptor.fullyQualifiedMethod, context: serviceContext, ofKind: .client) { span in | ||
| span.addEvent("Sending request") | ||
| let response = try await next(request, context) | ||
| span.addEvent("Received response") |
There was a problem hiding this comment.
So this is interesting because we might be streaming here. We should definitely rename this to something like Received response end or similar. Next, should we provide a mode of this interceptor that emits events on every single message?
There was a problem hiding this comment.
Agreed about changing the event name. With regards to emitting events on each message - I agree it could be useful. I think the only way of doing this is by creating a new RPCWriter wrapping the passed one, so I'll take this approach for both client and server interceptors.
| using: ClientRequestInjector() | ||
| ) | ||
|
|
||
| return try await tracer.withSpan(context.descriptor.fullyQualifiedMethod, context: serviceContext, ofKind: .client) { span in |
There was a problem hiding this comment.
Are there other attributes that we might want to add to the span from the request?
There was a problem hiding this comment.
Can't think of anything else that would be universal to all requests
...reTests/Call/Client/Internal/ClientRPCExecutorTestSupport/ClientRPCExecutorTestHarness.swift
Outdated
Show resolved
Hide resolved
Motivation
With v2 of gRPC, we want to provide tracing interceptors for both the client and server sides.
Modifications
This change adds tracing interceptors implementations for both the client and the server, using
swift-distributed-tracing.Result
We've got tracing interceptors.