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

64bit connection id may break slow query detail #791

Closed
breezewish opened this issue Nov 6, 2020 · 12 comments · Fixed by #793
Closed

64bit connection id may break slow query detail #791

breezewish opened this issue Nov 6, 2020 · 12 comments · Fixed by #793
Labels
Milestone

Comments

@breezewish
Copy link
Member

Bug Report

Please answer these questions before submitting your issue. Thanks!

What did you do?

TiDB generates 64bit connection id since pingcap/tidb#17649. In some cases, the slow query detail page cannot correctly handle this case due to JavaScript max int limitation.

What did you expect to see?

Slow query detail work correctly.

What did you see instead?

Slow query detail may throw error.

What version of TiDB Dashboard are you using (./tidb-dashboard --version)?

@baurine
Copy link
Collaborator

baurine commented Nov 7, 2020

Does it work by using string type in the response?

@unbyte
Copy link
Contributor

unbyte commented Nov 7, 2020

@kennytm
Copy link
Contributor

kennytm commented Nov 7, 2020

TBH I'd prefer using json-bigint + bignumber.js than changing the JSON type to string...

Or we could restrict the Global connection ID to use only 53 bits 🙃, reducing the range from 22(server ID) + 40(local conn) to 18(server ID) + 34(local conn).

@breezewish
Copy link
Member Author

Prefer json-bigint +1

We'd better not changing the global connection ID bit structure since this is a fault of JavaScript instead of connection ID.

@breezewish breezewish added this to the 4.0.x milestone Nov 9, 2020
@breezewish
Copy link
Member Author

breezewish commented Nov 11, 2020

Some discovery:

  • For parse usage, occurs when receiving server response, this problem can be gracefully handled by customizing transformResponse and introducing json-bigint.

  • For stringify usage, occurs when generating server request, looks like there is no way to use bignumber or json-bigint (?). The request is generated by the swagger client. It accepts TypeScript number signatures, and then use url.format to generate the actual URL. url.format internally use querystring.stringify, which only accepts primitive types instead of a bignumber object. Any thoughts?

Related discussions:

@kennytm
Copy link
Contributor

kennytm commented Nov 11, 2020

can we map the BigInt toString before passing to swagger?

@breezewish
Copy link
Member Author

breezewish commented Nov 11, 2020

can we map the BigInt toString before passing to swagger?

@kennytm We could. However this loses its advantage. Public interfaces are no longer semantically correct, server side API and Swagger specs both accept string instead of uint64 for fields that should be uint64.

In PR #793 I simply change uint64 to string, leaving a TODO, so that we can change it back when swagger and generated clients support BigInt. What do you think about this solution?

@kennytm
Copy link
Contributor

kennytm commented Nov 11, 2020

(filed nodejs/node#36080 for bigint support in querystring)

(but why are we using nodejs?)

@breezewish
Copy link
Member Author

(filed nodejs/node#36080 for bigint support in querystring)

(but why are we using nodejs?)

We are using Webpack, so that in addition to NodeJs support, we also need to expect browser polyfills to work correctly.

According to https://medium.com/@sanchit3b/how-to-polyfill-node-core-modules-in-webpack-5-905c1f5504a0, url and querystring will be mapped to corresponding module in npm, e.g. https://www.npmjs.com/package/querystring

@kennytm
Copy link
Contributor

kennytm commented Nov 11, 2020

ok. looks like even if we get nodejs/node#36080 fixed it will take a long time for us to support older nodejs versions in other dev environments 😅

@unbyte
Copy link
Contributor

unbyte commented Nov 11, 2020

Since https://github.com/tc39/proposal-bigint is a very new feature, neither the surrounding ecology nor Polyfill is mature. Once bigint is used, it needs to introduce a lot of additional processing code to pollute the workflow and code.
Therefore, I prefer to use string. Anyway, we did not change the nature of its numerical value. We only converted it into string for the display in frontend. This conversion is transparent to users.

@breezewish
Copy link
Member Author

breezewish commented Nov 11, 2020

@unbyte Yes. The only defect is that it makes Dashboard server API dirty. This API is internal for now but may be open to public in future. I hope one day we can change it back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants