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

*: add client trace info through session variable #51539

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

taloric
Copy link

@taloric taloric commented Mar 6, 2024

What problem does this PR solve?

Issue Number: close #51144

Problem Summary:

  • enable trace info parsed from client and use it to start a full trace in server
  • it's a implement in current version like what's *: Integrate timeline tracing with TiKV #19557 have done, it supports using tidb_trace_id as session variable to log trace infos.

What changed and how does it work?

  • add tidb_trace_id as session variable and log it into slow_log when a query is too slow
  • allow client connection use set tidb_trace_id= xxxx in a session, then start a span trace inherited from client trace id.
  • update jaeger-client-go to v2.26.0, which supports pass tracing options from Environment Variables

Check List

Tests

  • Unit test

  • Integration test

  • Manual test (add detailed scripts or steps below)

  • No need to test

    • I checked and no code files have been changed.
  • test results:

    • base on a java spring boot demo
    • base on deepflow as backend display

image

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Allow distributed tracing start from client applications through tracing info passed by session variable.

@sre-bot
Copy link
Contributor

sre-bot commented Mar 6, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

ti-chi-bot bot commented Mar 6, 2024

Welcome @taloric!

It looks like this is your first PR to pingcap/tidb 🎉.

I'm the bot to help you request reviewers, add labels and more, See available commands.

We want to make sure your contribution gets all the attention it needs!



Thank you, and welcome to pingcap/tidb. 😃

@ti-chi-bot ti-chi-bot bot added needs-ok-to-test size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 6, 2024
Copy link

ti-chi-bot bot commented Mar 6, 2024

Hi @taloric. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link

tiprow bot commented Mar 6, 2024

Hi @taloric. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@taloric
Copy link
Author

taloric commented Mar 18, 2024

@Defined2014 hello, I've already fix all failed test, but still remain a 404 not found error during download the new go module file which I updated in go.mod and bazel deps.
Isn't it suppose to upload zip files to the repository automatically in the github workflow? I found none of this description in contribution. Anything I've missed? Any help would be appreciate, tks~

@Defined2014
Copy link
Contributor

/retest

@Defined2014
Copy link
Contributor

@Defined2014 hello, I've already fix all failed test, but still remain a 404 not found error during download the new go module file which I updated in go.mod and bazel deps. Isn't it suppose to upload zip files to the repository automatically in the github workflow? I found none of this description in contribution. Anything I've missed? Any help would be appreciate, tks~

Maybe need run bazel run //cmd/mirror -- --mirror --upload

Copy link

codecov bot commented Mar 18, 2024

Codecov Report

Attention: Patch coverage is 48.07692% with 27 lines in your changes missing coverage. Please review.

Project coverage is 55.9681%. Comparing base (5b5915b) to head (502ff87).
Report is 60 commits behind head on master.

Additional details and impacted files
@@                Coverage Diff                @@
##             master     #51539         +/-   ##
=================================================
- Coverage   72.9011%   55.9681%   -16.9330%     
=================================================
  Files          1549       1670        +121     
  Lines        436313     608494     +172181     
=================================================
+ Hits         318077     340563      +22486     
- Misses        98726     244624     +145898     
- Partials      19510      23307       +3797     
Flag Coverage Δ
integration 37.1025% <38.4615%> (?)
unit 71.7687% <48.0769%> (-0.1703%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 52.9656% <ø> (ø)
parser ∅ <ø> (∅)
br 52.6027% <ø> (+6.7120%) ⬆️

@Defined2014
Copy link
Contributor

/retest

@ti-chi-bot ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 2, 2024
@ti-chi-bot ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2024
Copy link

ti-chi-bot bot commented Jul 8, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign wjhuang2016, yudongusa for approval, ensuring that each of them provides their approval before proceeding. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

ti-chi-bot bot commented Jul 9, 2024

@taloric: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-br-integration-test d01fa3f link true /test pull-br-integration-test
pull-lightning-integration-test d01fa3f link true /test pull-lightning-integration-test

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@taloric
Copy link
Author

taloric commented Jul 13, 2024

/retest

@taloric
Copy link
Author

taloric commented Jul 14, 2024

/assign @wjhuang2016 @yudongusa

Hello, it's been a long time but I've fixed all test cases, plz check if this issue was acceptable :)

Copy link

ti-chi-bot bot commented Jul 14, 2024

@taloric: GitHub didn't allow me to assign the following users: yudongusa.

Note that only pingcap members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @wjhuang2016 @yudongusa

Hello, it's been a long time but I've fixed all test cases, plz check if this issue was acceptable :)

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link
Member

@wjhuang2016 wjhuang2016 left a comment

Choose a reason for hiding this comment

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

Introducing a new variable needs to be approved by the PM, I'll let him check it later.

@ti-chi-bot ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2024
Copy link

ti-chi-bot bot commented Jul 20, 2024

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test release-note size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support distributed tracing between user-application with tidb and other componentes
4 participants