Skip to content

CURATOR-726: Improve tracing for certain operations#513

Merged
kezhuw merged 5 commits intoapache:masterfrom
HoustonPutman:tracing-fixes
Dec 20, 2024
Merged

CURATOR-726: Improve tracing for certain operations#513
kezhuw merged 5 commits intoapache:masterfrom
HoustonPutman:tracing-fixes

Conversation

@HoustonPutman
Copy link
Copy Markdown
Contributor

@HoustonPutman HoustonPutman commented Dec 16, 2024

https://issues.apache.org/jira/browse/CURATOR-726

In Apache Solr, we are trying to refactor all of our custom ZK interaction logic to use Curator. One of the things we do is implement metrics on our interactions with ZK. Curator allows us to do this with Tracing and CuratorListeners. However there are two things that don't give us the information we need.

  1. The "Get Children" command trace does not give the information needed for the number of Children fetched.
  2. The "Multi Transaction" command trace:
    • The trace does not give the number of commands that are in the transaction, which is something we track currently.
    • The MultiTransaction trace is not an AdvancedTrace, it's a normal Trace for some reason. AdvancedTracerDrivers cannot read regular traces, so we can't even see these. We need to update Curator to use an AdvancedTrace for this command.

Therefore in this PR, I have added the two pieces of information (# of Children fetched and # of transactions) and also converted the MultiTransaciton to use an AdvancedTrace.

tisonkun
tisonkun approved these changes Dec 18, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Thank you!

@tisonkun tisonkun requested a review from kezhuw December 18, 2024 03:27
kezhuw
kezhuw approved these changes Dec 19, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@kezhuw kezhuw left a comment

Choose a reason for hiding this comment

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

LGTM

I think we probably should trace also opcode in future. The name is not good for programmatical decision.

Comment thread curator-framework/src/main/java/org/apache/curator/framework/imps/GetChildrenBuilderImpl.java
Comment thread curator-framework/src/main/java/org/apache/curator/framework/imps/GetChildrenBuilderImpl.java
Comment thread curator-client/src/main/java/org/apache/curator/drivers/OperationTrace.java Outdated
@HoustonPutman
Copy link
Copy Markdown
Contributor Author

HoustonPutman commented Dec 19, 2024

I think we probably should trace also opcode in future. The name is not good for programmatical decision.

100% agreed! CuratorEventType already exists, so we can repurpose it for this as well.

@HoustonPutman
Copy link
Copy Markdown
Contributor Author

HoustonPutman commented Dec 19, 2024

I think we probably should trace also opcode in future. The name is not good for programmatical decision.

100% agreed! CuratorEventType already exists, so we can repurpose it for this as well.

Well that's very frustrating. CuratorEventType is in curator-framework, while OperationTrace is in curator-client, so OperationTrace cannot have a CuratorEventType variable...

kezhuw
kezhuw reviewed Dec 19, 2024
View reviewed changes
Comment thread curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorEventImpl.java Outdated
@kezhuw
Copy link
Copy Markdown
Member

kezhuw commented Dec 19, 2024

I think we probably should trace also opcode in future.

I guess this require a bunch of changes and worth a separated issue. Let's leave it for future improvement.

@kezhuw kezhuw merged commit eb99124 into apache:master Dec 20, 2024
@HoustonPutman HoustonPutman deleted the tracing-fixes branch March 7, 2025 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kezhuw kezhuw kezhuw approved these changes

@tisonkun tisonkun tisonkun 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.

3 participants

@HoustonPutman @kezhuw @tisonkun