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

Diagnose report: fix disk total query #577

Closed
wants to merge 1 commit into from

Conversation

YKG
Copy link

@YKG YKG commented May 28, 2020

Signed-off-by: Kaige Ye [email protected]

This PR is intended to close this issue: tidb-challenge-program/bug-hunting-issue#101

Explanation:

What I changed in this PR is to delete this condition:

AND `DEVICE_NAME` NOT LIKE '/dev/loop%' AND (`DEVICE_NAME` LIKE '/dev%' or `DEVICE_NAME` LIKE 'sda%' or`DEVICE_NAME` LIKE 'nvme%') group by instance,device_name,value

after beautify:

AND `DEVICE_NAME` NOT LIKE '/dev/loop%' 
AND (`DEVICE_NAME` LIKE '/dev%' 
  or `DEVICE_NAME` LIKE 'sda%' 
  or `DEVICE_NAME` LIKE 'nvme%') 
group by instance,device_name,value
  • first of all, the last group by is useless, since each row the result will just be saved into a map
  • DEVICE_NAME LIKE '/dev%' is useless, because the system info retrived from RPC doesn't not include this prefix
  • DEVICE_NAME LIKE 'sda%' or DEVICE_NAME LIKE 'nvme%' is not enough, because sdb%/sdc%/vda% are also valid, more than that, we should not have any assumption on the name
  • For the DEVICE_NAME NOT LIKE '/dev/loop%' part, if it should be hidden in dashboard, the right place to do that is to filter it in the datasouce(i.e. these info should be filtered in TiDB/PD/TiKV/TiFlash server info collector)

Another thing is, from a designer's perspective, dashboard SHOULD NOT filter any thing the underline services' report. If anything should be filtered, they should be filtered in the underline services, or these infomation will become junk info, no one use it. The only situation I can imagine to do the filter in dashboard is to do some temporary hack.

@baurine
Copy link
Collaborator

baurine commented May 28, 2020

@crazycs520 PTAL

@breezewish
Copy link
Member

cool. How will be the report looks like after this change?

@YKG
Copy link
Author

YKG commented Jun 2, 2020

@breeswish Here is the report after this change:

Screenshot from 2020-06-02 17-24-08

@YKG
Copy link
Author

YKG commented Jun 2, 2020

And now I confirmed that TiKV does't have a correct disk info collector in v4.0.0-rc.2 which I have mentioned in this bug report. The root cause is the sysinfo doesn't have correct implementation, it's sysinfo again. I also confirmed that it has been fixed in the latest sysinfo.

@breezewish
Copy link
Member

@crazycs520 How is this PR differs from yours?

@breezewish
Copy link
Member

Thanks for the contribution! I'm going to close this PR since @crazycs520 proposed a similar one and changed less.

@breezewish breezewish closed this Jun 28, 2020
@YKG
Copy link
Author

YKG commented Jun 29, 2020

@breeswish I reviewed #624, but I'd like to copy my old comments above to here:

  • first of all, the last group by is useless, since each row the result will just been saved into a map
    ...
  • For the DEVICE_NAME NOT LIKE '/dev/loop%' part, if it should be hidden in dashboard, the right place to do that is to filter it in the datasouce(i.e. these info should be filtered in TiDB/PD/TiKV/TiFlash server info collector)

@breezewish
Copy link
Member

@YKG Thanks for the comment! For the /dev/loop I think this is the right place to filter, since the hardware table only displays hardwares and does not care about what kind of data is actually useful. It's up to user to decide what they need. While in the diagnostics report we specifically want to minimize the data we provide to users.

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

Successfully merging this pull request may close these issues.

3 participants