-
Notifications
You must be signed in to change notification settings - Fork 903
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue #1781: Add traces/zipkin support to simplify troubleshooting of slow requests #1782
Conversation
…ing of slow requests
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.
This is a great change.
However is it possible to not tie to a specific tracing library in the code. I would suggest adding interceptor
interface to intercept bookkeeper operations, and tracing can be one of interceptor implementation.
for example:
- pulsar interceptors: https://github.com/apache/pulsar/wiki/PIP-23%3A-Message-Tracing-By-Interceptors
- grpc interceptor: https://github.com/grpc/grpc-java/blob/master/core/src/main/java/io/grpc/ClientInterceptor.java
- tracing interceptor: https://github.com/grpc-ecosystem/grpc-opentracing/blob/master/java/src/main/java/io/opentracing/contrib/ServerTracingInterceptor.java#L31
with an interceptor interface, it will be easier to configure the behavior on client with builder method:
BookKeeperBuilder intercept(Interceptor... interceptors);
I've seen in many places this is using multiple builder object to track operations in different places. I would like to make sure there is no overhead (like in objects allocations) if the tracing feature is disabled. |
@merlimat I agree with this concern. @sijie Let me make sure that I understood you correctly. |
yes.
Tracer is one type of interface, which user can implement their tracer logic. However I think a more generic to intercept bookkeeper operations would be better for extensibility, since users can do things beyond tracing, and doesn't have to be tied to a Tracer/OpenTracing interface. |
as in any possible step? i.e. traces pass context through executor etc, not sure what value it provides outside of the context of higher level trace. Either way, one can create Tracer that does some internal magic with traces, register that Tracer as a global Tracer. as in very specific points of bookie/client process (add start-finish, remote call start-finish etc)? I am not familiar with pulsar, I took a very brief look at https://github.com/apache/pulsar/blob/7a92111fe7824d422d1739abd06d85955b652212/pulsar-broker/src/test/java/org/apache/pulsar/client/api/InterceptorsTest.java where interceptors seem to be events that happen on start/finish of some operations. TL;DR: |
@dlg99 : interceptor is usually a pattern that keep the core business logic separated from any other argumented logics (e.g. tracing, filtering, monitoring). so argumented logic can be easier to unit test rather than coupling with the core runtime. All what we are caring here is client operations (e.g. metadata operations, data operations) and server side operations (e.g. storage operations, journal operation) and executor operations. If we define some sort of interceptor interfaces for those key operations, it is easier for us to add a tracing interceptor. There are a few benefits have interceptors:
Based on these considerations, I think a better approach will be:
Other comments on your comment:
An interceptor interface doesn't limit the flexibility. You are still using open tracing api to set parent/child relationship.
A better way to think about interceptor vs tracing. Interceptor is the interface, while tracing is the implementation. I am not saying interceptors replacing tracing. what I am suggesting is a common interceptor interface is allowing more extensibility, rather than directly sticking to one tracing api.
End-to-end tracing is one of the use cases when bookkeeper is used as one piece in the whole system. Context propagation will be important in such case, providing interceptors rather than directly implementing tracing will be more flexible for people to do proper context propagation and use proper tracing framework (e.g. open source, in house). Allow any in-house implementations that do tracing but doesn't use any open source tracing api. |
I think I have an idea what to do, it won't be exactly interceptors fw but rather an extension of current metrics. I'll do the POC as time allows and we'll look at it. |
Descriptions of the changes in this PR:
Instrumented bookie and the client with tracing, added configuration to enable tracing.
Motivation
Tracing compliments metrics framework by enabling one's deep dive into performance details of single request. Zipkin provides UI to view such traces though there are other implementations.
Opentracing provides framework for tracing that can be used with various tracing backends, zipkin included.
Changes
Instrumented bookie and the client with tracing, added configuration to enable tracing.
Added unit tests.
Master Issue: #1781