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

telemetry: Add telemetry information about builtin functions usage #26234

Merged
merged 13 commits into from
Jul 21, 2021

Conversation

leiysky
Copy link
Contributor

@leiysky leiysky commented Jul 14, 2021

What problem does this PR solve?

Issue Number: close #25947

Problem Summary:

What is changed and how it works?

Proposal: xxx

What's Changed:

How it Works:

Use a map[string]uint32 to record builtin functions usage information in each telemetry window.

Since for each expression in a single SQL query will be accessed by expressionRewriter, we can record the builtin functions usage information as soon as expressionRewriter.newFunction is invoked. Then we can submit the result to GlobalBuiltinFunctionUsageCollector after executing the query.

After adding this field, the telemetry data will look like:

{
    "beginAt": "2021-07-14T13:42:11.7490453+08:00",
    "executeCount": 0,
    "tiFlashUsage": {
        "pushDown": 0,
        "exchangePushDown": 0
    },
    "coprCacheUsage": {
        "gte0": 0,
        "gte1": 0,
        "gte10": 0,
        "gte20": 0,
        "gte40": 0,
        "gte80": 0,
        "gte100": 0
    },
    "SQLUsage": {
        "total": 0,
        "type": {}
    },
    "builtinFunctionsUsage": {
        "EQInt": 20,
        "EQString": 1,
        "GTInt": 545,
        "GTTime": 548,
        "InString": 1,
        "LTInt": 11,
        "LeftUTF8": 1,
        "PlusInt": 261,
        "SHA2": 1,
        "UnaryNotInt": 1
    }
}

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

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

  • Add telemetry information about builtin functions usage

@ti-chi-bot ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 14, 2021
@leiysky
Copy link
Contributor Author

leiysky commented Jul 14, 2021

/run-all-tests

1 similar comment
@leiysky
Copy link
Contributor Author

leiysky commented Jul 14, 2021

/run-all-tests

@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Jul 14, 2021
@tisonkun tisonkun self-requested a review July 14, 2021 06:44
@leiysky
Copy link
Contributor Author

leiysky commented Jul 14, 2021

/cc @kaixu120811

PTAL.

@ti-chi-bot ti-chi-bot requested a review from kaixu120811 July 14, 2021 08:04
Copy link
Contributor

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

LGTM.

@ti-chi-bot
Copy link
Member

@tisonkun: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

LGTM.

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 ti-community-infra/tichi repository.

@leiysky
Copy link
Contributor Author

leiysky commented Jul 14, 2021

/cc @breeswish

@ti-chi-bot ti-chi-bot requested a review from breezewish July 14, 2021 08:52
Copy link
Contributor

@LittleFall LittleFall left a comment

Choose a reason for hiding this comment

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

LGTM�

@ti-chi-bot
Copy link
Member

@LittleFall: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

LGTM�

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 ti-community-infra/tichi repository.

@LittleFall
Copy link
Contributor

test need fix:

[2021-07-15T08:07:27.464Z] FAIL: planbuilder_test.go:144: testPlanBuilderSuite.TestDisableFold
[2021-07-15T08:07:27.464Z] 
[2021-07-15T08:07:27.464Z] planbuilder_test.go:184:
[2021-07-15T08:07:27.464Z]     c.Assert(rewritenArg, FitsTypeOf, expectedArg)
[2021-07-15T08:07:27.464Z] ... obtained *expression.Constant = &expression.Constant{Value:types.Datum{k:0x4, decimal:0x0, length:0x0, i:-4621415532179685754, collation:"", b:[]uint8(nil), x:interface {}(nil)}, RetType:(*types.FieldType)(0xc00080fc20), DeferredExpr:expression.Expression(nil), ParamMarker:(*expression.ParamMarker)(nil), hashcode:[]uint8(nil), collationInfo:expression.collationInfo{coer:5, coerInit:true, charset:"", collation:"", flen:0}} ("-0.45990349068959124")
[2021-07-15T08:07:27.464Z] ... sample *expression.ScalarFunction = &expression.ScalarFunction{FuncName:model.CIStr{O:"", L:""}, RetType:(*types.FieldType)(nil), Function:expression.builtinFunc(nil), hashcode:[]uint8(nil)} (%!q(PANIC=String method: runtime error: invalid memory address or nil pointer dereference))

@leiysky
Copy link
Contributor Author

leiysky commented Jul 16, 2021

/run-all-tests

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

@Vancior: Request changes is only allowed for the reviewers in list.

In response to this:

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 ti-community-infra/tichi repository.

Copy link

@Vancior Vancior left a comment

Choose a reason for hiding this comment

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

LGTM.
@lucklove

@lucklove
Copy link
Member

/sig tiup

Copy link
Member

@lucklove lucklove left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 21, 2021
@leiysky
Copy link
Contributor Author

leiysky commented Jul 21, 2021

/remove-sig tiup

@leiysky
Copy link
Contributor Author

leiysky commented Jul 21, 2021

/remove-sig sql-infra

@ti-chi-bot ti-chi-bot removed the sig/sql-infra SIG: SQL Infra label Jul 21, 2021
Copy link
Contributor

@LittleFall LittleFall left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • LittleFall
  • lucklove

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 21, 2021
@LittleFall
Copy link
Contributor

/merge

1 similar comment
@leiysky
Copy link
Contributor Author

leiysky commented Jul 21, 2021

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 3ea15b4

1 similar comment
@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 3ea15b4

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 21, 2021
@leiysky
Copy link
Contributor Author

leiysky commented Jul 21, 2021

/sig tiup

@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Jul 21, 2021
@dveeden
Copy link
Contributor

dveeden commented Jul 21, 2021

Please add details about this to https://docs.pingcap.com/tidb/stable/telemetry once merged

@ti-chi-bot ti-chi-bot merged commit 126c9b4 into pingcap:master Jul 21, 2021
@leiysky leiysky deleted the feature/builtinfunctions-telemetry branch July 21, 2021 10:24
@lilyjazz
Copy link
Member

doc pr: pingcap/docs-cn#6841

@breezewish
Copy link
Member

@lzmhhh123 is performing additional review for this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/sql-infra SIG: SQL Infra size/L Denotes a PR that changes 100-499 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add builtinfunction usage statistics
9 participants