Skip to content
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

[to #477] Change SlowLog interface to align with tracing #485

Merged
merged 31 commits into from
Feb 10, 2022

Conversation

peng1999
Copy link
Member

@peng1999 peng1999 commented Jan 19, 2022

Signed-off-by: Peng Guanwen pg999w@outlook.com

What problem does this PR solve?

Issue Number: to #477
Problem Description: See Proposal

What is changed and how it works?

  • Interfaces in SlowLog and SlowLogSpan are adjusted.
  • Time measuring in SlowLogImpl and SlowLogSpanImpl are refined.
  • Log outputs are adjusted to align with tracing.

Code changes

  • Has interface methods change

Check List for Tests

This PR has been tested by the at least one of the following methods:

  • Unit test TBD

Related changes

  • NO related changes

Signed-off-by: Peng Guanwen <pg999w@outlook.com>
ref tikv#477

Signed-off-by: Peng Guanwen <pg999w@outlook.com>
Signed-off-by: Peng Guanwen <pg999w@outlook.com>
Signed-off-by: Peng Guanwen <pg999w@outlook.com>
@peng1999 peng1999 marked this pull request as ready for review January 21, 2022 06:29
@peng1999
Copy link
Member Author

Signed-off-by: Peng Guanwen <pg999w@outlook.com>
Signed-off-by: Peng Guanwen <pg999w@outlook.com>
@andylokandy
Copy link
Contributor

I think we also need to fill the trace_context in kvrpcpb.Context.

Signed-off-by: Peng Guanwen <pg999w@outlook.com>
Signed-off-by: Peng Guanwen <pg999w@outlook.com>
Signed-off-by: Peng Guanwen <pg999w@outlook.com>
Signed-off-by: Peng Guanwen <pg999w@outlook.com>
return Kvrpcpb.Context.newBuilder(context)
.setTraceContext(
Tracepb.TraceContext.newBuilder()
.setDurationThresholdMs((int) slowLog.getThresholdMS())
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should set the threshold for tikv less than the client. Maybe half of the client threshold? @marsishandsome what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we make the factor(client threshold / tikv threshold) configurable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds reasonable

Signed-off-by: Peng Guanwen <pg999w@outlook.com>
Copy link
Collaborator

@marsishandsome marsishandsome left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@marsishandsome marsishandsome left a comment

Choose a reason for hiding this comment

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

How can users disable slowlog? Is there a switch?

Signed-off-by: Peng Guanwen <pg999w@outlook.com>
Signed-off-by: Peng Guanwen <pg999w@outlook.com>
Signed-off-by: Peng Guanwen <pg999w@outlook.com>
Signed-off-by: Peng Guanwen <pg999w@outlook.com>
Signed-off-by: Peng Guanwen <pg999w@outlook.com>
Signed-off-by: Peng Guanwen <pg999w@outlook.com>
Signed-off-by: Peng Guanwen <pg999w@outlook.com>
Signed-off-by: Peng Guanwen <pg999w@outlook.com>
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason
Copy link
Member

/merge

@ti-srebot
Copy link
Collaborator

/run-all-tests

@ti-srebot
Copy link
Collaborator

@peng1999 merge failed.

Signed-off-by: Peng Guanwen <pg999w@outlook.com>
@zz-jason
Copy link
Member

/merge

@ti-srebot
Copy link
Collaborator

/run-all-tests

Copy link
Member

@iosmanthus iosmanthus left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason merged commit 7122903 into tikv:master Feb 10, 2022
@peng1999 peng1999 deleted the trace-slow branch February 10, 2022 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants