Skip to content

ZOOKEEPER-3432 Improving zookeeper trace for performance and scalability#994

Open
gmcatsf wants to merge 1 commit intoapache:masterfrom
gmcatsf:ZOOKEEPER-3432
Open

ZOOKEEPER-3432 Improving zookeeper trace for performance and scalability#994
gmcatsf wants to merge 1 commit intoapache:masterfrom
gmcatsf:ZOOKEEPER-3432

Conversation

@gmcatsf
Copy link

@gmcatsf gmcatsf commented Jun 20, 2019

Added TraceLogger to send traces to external process via netty. Trace has a strongly typed schema defined inside TraceField.

@maoling
Copy link
Member

maoling commented Jun 21, 2019

@gmcatsf awesome work.

  • Cloud you plz add some details about how we visit the tracing chain？
  • In my knowledge, Writing the filter and reporting the tracings to a third-party distributed tracing system(e.g zipkin) is a more simple way?

hanm
hanm reviewed Jun 22, 2019
View reviewed changes
Copy link
Contributor

@hanm hanm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice improvement as trace never really works as expected in ZK.

Very high level comments:

  • Might worth consider adding a feature switch to turn on / off the tracing.

  • This adds new system properties / configuration options - and these should be properly documented.

  • A documentation on how to set up end to end tracing would be great.

@hanm
Copy link
Contributor

hanm commented Jun 22, 2019

Writing the filter and reporting the tracings to a third-party distributed tracing system(e.g zipkin) is a more simple way?

My thoughts on this is, the approach this patch took and third party tracing integration is not mutually exclusive, and it's probably good to have a self contained tracing solution for ZK w/o 3rd party dependency.
It would be good to keep the option open though to consider potential integration to 3rd party system like openTracing while working on this patch.

@gmcatsf
Copy link
Author

gmcatsf commented Jun 24, 2019

@gmcatsf awesome work.

  • Cloud you plz add some details about how we visit the tracing chain？

This implementation is sending traces to external process and there is no persistence in the pull request. It is up to the external process how to persist those traces. We have a separate external process in c++ which receive those traces and then write them to scribe or any other framework.

  • In my knowledge, Writing the filter and reporting the tracings to a third-party distributed tracing system(e.g zipkin) is a more simple way?

I think it could co-exist with third-party tracing integration, though the code needs some re-factory like changing TraceLogger to interface and adding a trace logger factor to load different implementation. The reason behind current design is to export traces through a single stateful socket connection without no new third-party library dependency in zookeeper.

@gmcatsf
Copy link
Author

gmcatsf commented Jun 24, 2019

This is a nice improvement as trace never really works as expected in ZK.

Very high level comments:

  • Might worth consider adding a feature switch to turn on / off the tracing.

There is a system property named "zookeeper.disableTraceLogger" to turn off all tracing (this was added by Ben Reed when he was in FB).

  • This adds new system properties / configuration options - and these should be properly documented.
  • A documentation on how to set up end to end tracing would be great.

Where is the best place to add documentation? Or shall it be a separate doc?

@gmcatsf
Copy link
Author

gmcatsf commented Jun 24, 2019

Writing the filter and reporting the tracings to a third-party distributed tracing system(e.g zipkin) is a more simple way?

My thoughts on this is, the approach this patch took and third party tracing integration is not mutually exclusive, and it's probably good to have a self contained tracing solution for ZK w/o 3rd party dependency.
It would be good to keep the option open though to consider potential integration to 3rd party system like openTracing while working on this patch.

Adding TraceLoggerFactory and TraceLogger interface might help generalizing the implementation, please kindly let me know if I should make the change here.

@maoling
Copy link
Member

maoling commented Jun 25, 2019

@gmcatsf

Where is the best place to add documentation? Or shall it be a separate doc?

Search around, there's no good place to add this doc.You can create a separate doc：e.g zookeeperTracing.md

  • almost 3000 line changes, too lazy to read. D. I have the following concerns:
    • the trace has something like traceId, if so, how to generate it?
    • this trace feature don't have a switch?
    • the sendReport Thread is asyn? enabling this feature has any performance issue?

This implementation is sending traces to external process and there is no persistence in the pull request. It is up to the external process how to persist those traces.

  • is it not user-friendly? how to send traces to external process? what thing my external process need to do?
  • Overall, the trace implements should comply with the opentracing specfication(https://github.com/opentracing/opentracing-java)? Otw, too much work are exposed to the users.

@gmcatsf
Copy link
Author

gmcatsf commented Jun 26, 2019

@maoling I will add zookeeperTracing.md, please see my answers inline

  • almost 3000 line changes, too lazy to read. D. I have the following concerns:

    • the trace has something like traceId, if so, how to generate it?

There is no traceId for trace message, could you specify which line this refers to?

  • this trace feature don't have a switch?

TraceLogger can be turned off by ZooTrace.disableTraceLogger via system property zookeeper.disableTraceLogger. There is also an existing ZooTrace.traceMask which controls which traces to record.

  • the sendReport Thread is asyn? enabling this feature has any performance issue?

I assume you mean TraceLogger.TraceMessage.send()? this method is asynchronously adding trace into a queue with limited capacity and return immediately to avoiding slowing down server. Traces added to the queue may be dropped if queue overflows. Performance issue with existing ZooTrace is the exact reason for the pull request. Note the implementation uses only single socket+single thread, and we have been running this on hundreds of facebook production ensembles over one year without noticing performance impact.

This implementation is sending traces to external process and there is no persistence in the pull request. It is up to the external process how to persist those traces.

  • is it not user-friendly? how to send traces to external process? what thing my external process need to do?

The pull request includes a testing class named TraceLoggerServer which shows how this works, it may not be enough and I will enhance it to show how this external process should read traces. This the same as the comment from @hanm

OpenTracing have a span/span context data model, which is different from existing ZooTrace implementation of log messages. The focus of this pull request is to replace writing to log4j part with writing to external process through socket so that server performance wont be affected. Would it make sense to create a separate jira for OpenTracing?

@maoling
Copy link
Member

maoling commented Jun 26, 2019

@gmcatsf

  • Thanks for clarification and answering all my concerns.

  • Would it make sense to create a separate jira for OpenTracing

    Of course.I have a little knowledge about the zipkin, in the further,I can do the work to integrate with opentracing :D
    but integrating with opentracing has a low priority, in the current, we should put more effort on the new metric system.

  • give me some more time, to see how this patch can better adapt to the further opentracing implement.

@maoling
Copy link
Member

maoling commented Sep 25, 2019

  • I take another look at this PR. One of the most valuable thing of this PR is the pointcut of tracing,i.e. where to do the tracing.

  • I prefer to use the opentracing api to do this work, for the following benefits:

    • Use the standard terminology(e.g:traceId, span...) to help uses to have a better understanding of the implementation
    • Save a ton of codes to use the ready-made opentracing api, hide the details about how to generate the traceId, how to send the tracing, how to storage the tracing...
    • Easy to make users to integrate with their existing tracing system(zipkin, jaeger...) seamlessly
    • A wonderful ready-made web UI to show the tracing chain to analyse

  • @gmcatsf Are you free there day? If not, I can pick it up and this PR will still sign off your name.

  • I watch the etcd community is also doing the tracing feature currently(etcd PR-11179), but that one seems like logging, not tracing. I wish we can land this great feature before etcd, and do it much better :)

@lvfangmin
Copy link
Contributor

lvfangmin commented Nov 15, 2019

@maoling it makes sense to leave the possibility of integrating with 3rd party like openTracing, but it seems to me we can do that in a separate JIRA.

Most of the code here are focusing on where we collect and generate the trace event, it's not that much about how to send the tracing, we can adapt that to the openTracing later.

Given we've already running this on prod for a long time, I would suggest to keep minimal change on this for now and improve it separately, what's your idea?

@gmcatsf can you rebase this feature and keep this up to date?

eolivelli
eolivelli reviewed Nov 15, 2019
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a first pass.
It looks good, but this patch is huge to review.
It would be great to split it a little.
I will do a second pass as soon as possible

@anmolnar
Copy link
Contributor

anmolnar commented Dec 7, 2019

I'm probably already a dinosaur, because logging in general, especially logging to a separate process/server is just syslog. Full stop. (and forks: rsyslog, syslog-ng, etc.) I'm sure it can be easily achieved by log4j as well.

However I like the approach in this patch especially if it's already proven stable in production, so I'm not against committing a solution which already works nicely.

I agree with @hanm 's suggestions: documentation is super critical here and an on/off switch would also be beneficial.

@gmcatsf This patch has been outstanding for a long time. Any plans on rebasing? You definitely want to do it sooner rather than later, if you want to hit 3.6.0.

@asf-ci
Copy link

asf-ci commented Dec 19, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build-maven/1758/

Build result: FAILURE

[...truncated 828.13 KB...][JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/pom.xml to org.apache.zookeeper/parent/3.7.0-SNAPSHOT/parent-3.7.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-recipes/zookeeper-recipes-lock/pom.xml to org.apache.zookeeper/zookeeper-recipes-lock/3.7.0-SNAPSHOT/zookeeper-recipes-lock-3.7.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-contrib/pom.xml to org.apache.zookeeper/zookeeper-contrib/3.7.0-SNAPSHOT/zookeeper-contrib-3.7.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-client/pom.xml to org.apache.zookeeper/zookeeper-client/3.7.0-SNAPSHOT/zookeeper-client-3.7.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-jute/pom.xml to org.apache.zookeeper/zookeeper-jute/3.7.0-SNAPSHOT/zookeeper-jute-3.7.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-jute/target/zookeeper-jute-3.7.0-SNAPSHOT.jar to org.apache.zookeeper/zookeeper-jute/3.7.0-SNAPSHOT/zookeeper-jute-3.7.0-SNAPSHOT.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-jute/target/zookeeper-jute-3.7.0-SNAPSHOT-tests.jar to org.apache.zookeeper/zookeeper-jute/3.7.0-SNAPSHOT/zookeeper-jute-3.7.0-SNAPSHOT-tests.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-jute/target/zookeeper-jute-3.7.0-SNAPSHOT-sources.jar to org.apache.zookeeper/zookeeper-jute/3.7.0-SNAPSHOT/zookeeper-jute-3.7.0-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-jute/target/zookeeper-jute-3.7.0-SNAPSHOT-javadoc.jar to org.apache.zookeeper/zookeeper-jute/3.7.0-SNAPSHOT/zookeeper-jute-3.7.0-SNAPSHOT-javadoc.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-recipes/zookeeper-recipes-queue/pom.xml to org.apache.zookeeper/zookeeper-recipes-queue/3.7.0-SNAPSHOT/zookeeper-recipes-queue-3.7.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-docs/pom.xml to org.apache.zookeeper/zookeeper-docs/3.7.0-SNAPSHOT/zookeeper-docs-3.7.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-docs/target/zookeeper-docs-3.7.0-SNAPSHOT.jar to org.apache.zookeeper/zookeeper-docs/3.7.0-SNAPSHOT/zookeeper-docs-3.7.0-SNAPSHOT.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-docs/target/zookeeper-docs-3.7.0-SNAPSHOT-tests.jar to org.apache.zookeeper/zookeeper-docs/3.7.0-SNAPSHOT/zookeeper-docs-3.7.0-SNAPSHOT-tests.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-docs/target/zookeeper-docs-3.7.0-SNAPSHOT-sources.jar to org.apache.zookeeper/zookeeper-docs/3.7.0-SNAPSHOT/zookeeper-docs-3.7.0-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-recipes/pom.xml to org.apache.zookeeper/zookeeper-recipes/3.7.0-SNAPSHOT/zookeeper-recipes-3.7.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-client/zookeeper-client-c/pom.xml to org.apache.zookeeper/zookeeper-client-c/3.7.0-SNAPSHOT/zookeeper-client-c-3.7.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-contrib/zookeeper-contrib-rest/pom.xml to org.apache.zookeeper/zookeeper-contrib-rest/3.7.0-SNAPSHOT/zookeeper-contrib-rest-3.7.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-assembly/pom.xml to org.apache.zookeeper/zookeeper-assembly/3.7.0-SNAPSHOT/zookeeper-assembly-3.7.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-contrib/zookeeper-contrib-zooinspector/pom.xml to org.apache.zookeeper/zookeeper-contrib-zooinspector/3.7.0-SNAPSHOT/zookeeper-contrib-zooinspector-3.7.0-SNAPSHOT.pomchannel stopped[SpotBugs] Skipping execution of recorder since overall result is 'FAILURE'Setting status of fe704b0 to FAILURE with url https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build-maven/1758/ and message: 'FAILURE 'Using context: JenkinsMaven

@gmcatsf
ZOOKEEPER-3432 Improving zookeeper trace for performance and scalability
707c704 
Author:    Mocheng Guo <gmcatsf@gmail.com>
Date:      Wed Jun 19 17:25:35 2019 -0700

Added trace logger server to collect and write traces from trace logger.
Added bin/zkTraceServer.sh to run trace logger server
Added zookeeperTrace.md to describe how trace works.
@gmcatsf
Copy link
Author

gmcatsf commented Jan 3, 2020

@maoling @eolivelli @anmolnar I have rebased on top of master branch. document was already added based on @hanm comment. Please help review and see what further changes are necessary.

@eolivelli
Copy link
Contributor

eolivelli commented Jan 4, 2020

@lvfangmin @anmolnar @hanm ping

@anmolnar
Copy link
Contributor

anmolnar commented Jan 22, 2020

retest maven build

anmolnar
anmolnar approved these changes Jan 22, 2020
View reviewed changes
Copy link
Contributor

@anmolnar anmolnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. Very nice docs.

Is this patch targeted for 3.6.x?

@hanm Any more comments?

lvfangmin
lvfangmin approved these changes May 20, 2020
View reviewed changes
Copy link
Contributor

@lvfangmin lvfangmin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Mocheng is not at fb now, I'll rebase this diff when I have time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hanm hanm hanm left review comments

@lvfangmin lvfangmin lvfangmin approved these changes

@anmolnar anmolnar anmolnar approved these changes

@eolivelli eolivelli eolivelli approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@gmcatsf @maoling @hanm @lvfangmin @anmolnar @asf-ci @eolivelli