Thrift instrumentation simplifications#410
Conversation
| private final Instrumenter<ThriftRequest, ThriftResponse> instrumenter; | ||
| private final ContextPropagators propagators; | ||
| @Nullable private final TTransport configuredTransport; | ||
| private final State state = new State(); |
There was a problem hiding this comment.
As far as I remember this was deliberate. The factory is used to create separate protocol instances for input and output, we wish to share state for these. This isn't a concurrency issue because the clients (including the async client) aren't thread safe and can only execute a single request at a time.
There was a problem hiding this comment.
After thinking more about this I'm not sure whether the shared state would always be safe. I reworked the client instrumentation so it isn't needed any more.
| // propagation field, span will be started in readMessageEnd() | ||
| if (field.id == ContextPropagationUtil.TRACE_CONTEXT_FIELD_ID && field.type == TType.MAP) { | ||
| Map<String, String> headers = ContextPropagationUtil.readHeaders(protocol); | ||
| Map<String, String> headers = ContextPropagationUtil.readHeaders(this); |
There was a problem hiding this comment.
I think I find it easier to reason when the underlying protocol is passed. When this is passed then overriding a protocol methods used in readHeaders could have unexpected results.
2 participants
Simplifications for review on top of open-telemetry#18405. Each commit is a separate refactor:
ThriftRequestAccess/ThriftResponseAccessindirection — exposeThriftRequestconstructors andThriftResponse.FAILEDdirectly.endSpanflag and circular construction-vs-registration order inClientProtocolDecorator/ClientCallContext.StateperClientProtocolDecorator.Factory.getProtocol(...)call (was shared, concurrency bug for async).protocolfield inClientProtocolDecoratorandServerInProtocolDecorator.ThriftRpcAttributesGetter.getErrorType: switch on the cachederrorCodeinstead of callinggetType()twice.AsyncMethodCallbackUtil.wrap: drop dead@Nullable+ null-check (callers null-check first).ThriftTelemetry.wrapAsyncClient: keep separateInvocationTargetExceptionandThrowablecatches but callgetCause()once.ThriftSingletons.thriftServiceName(Class<?>)to dedupe thegetDeclaringClass()unwrap in both javaagent ConstructorAdvices.