Skip to content

Conversation

@rushilenekar20
Copy link
Contributor

@rushilenekar20 rushilenekar20 commented Apr 13, 2022

Hey ,
I am suggesting a flag for diskstats collector named as --collector.diskstats.allowed-devices=""

Problem : Requirement for only allowed-devices flag in diskstats collector.

Description :
There is " ignored-devices " flag for diskstats collector which ignores stats of devices .

But in our organisation we faced a problem where Operating System create some devices at runtime whose names are unknown to us . So we were unable to ignored those devices .

As negetive lookahead is not supported in Go language because it conflicts with the O(n)-time guarantees of the library.so, we were unable to ignored those devices.

I suggest to have a flag for only allowed-devices for diskstats collector .

Here are some screenshots of the solution for this problem

  1. without using allowed-device flag
    Screenshot (6)

there are multiple device sda , dm , sr0 .Lets assume sda and sr0 devices were unknown to us and they are created at runtime and we dont want to show metrics of those devices . we only want to show metrics for dm devices.

2)allowed-device flag
Screenshot (8)

  1. Use case
    Screenshot (7)

only dm devices are allowed

@discordianfish
Copy link
Member

Makes sense but what about renaming the netdev filter to devFilter or something and use it here as well? https://github.com/prometheus/node_exporter/blob/master/collector/netdev_filter.go

@rushilenekar20 rushilenekar20 force-pushed the rushikesh branch 2 times, most recently from 3ebf74e to 1cd2a5c Compare April 20, 2022 10:36
@rushilenekar20
Copy link
Contributor Author

Makes sense but what about renaming the netdev filter to devFilter or something and use it here as well? https://github.com/prometheus/node_exporter/blob/master/collector/netdev_filter.go

I have commited one change . I tried to solve this current problem using netdev_filter.go . By renaming of netdev_filter to devFilter , i don't know what will be the consequences to another collector which are using netdev filter .

Signed-off-by: Vitaly Zhuravlev <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

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

Let's get this consistent with the netdev flags. So similar like there, we should:

  1. rename this to device-include
  2. add new flag device-exclude
  3. Prefix the description of collector.diskstats.ignored-devices with "DEPRECATED: Use collector.diskstats.device-exclude" and add .Hidden() before String()
  4. Support excluding by setting either device-exclude but warn if people use the now deprecated flag

Basically: https://github.com/prometheus/node_exporter/blob/master/collector/netdev_common.go#L33-L77

@discordianfish
Copy link
Member

Makes sense but what about renaming the netdev filter to devFilter or something and use it here as well? https://github.com/prometheus/node_exporter/blob/master/collector/netdev_filter.go

I have commited one change . I tried to solve this current problem using netdev_filter.go . By renaming of netdev_filter to devFilter , i don't know what will be the consequences to another collector which are using netdev filter .

Yeah you need to rename it in the other collectors but that should be straight forward.

Copy link
Member

@discordianfish discordianfish left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@discordianfish discordianfish requested a review from SuperQ April 28, 2022 09:55
beorn7 and others added 5 commits May 3, 2022 12:59
@rushilenekar20
Copy link
Contributor Author

Hey @SuperQ , We need this flag . Could you help me for this PR ?

@SuperQ
Copy link
Member

SuperQ commented May 17, 2022

Looks like this needs a rebase.

@SuperQ
Copy link
Member

SuperQ commented May 17, 2022

Also the tests are failing:

collector/diskstats_linux_test.go:52:3: undefined: ignoredDevices

@SuperQ
Copy link
Member

SuperQ commented May 19, 2022

I split out the netDevFilter out into a separate PR to simplify this change.

#2378

@SuperQ
Copy link
Member

SuperQ commented May 20, 2022

It looks like you've incorrectly rebased your branch, bringing in additional unrelated commits. Please fix up your commit history.

Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

I've merged #2378, please rebase and fix up the commit history on this change.

@SuperQ SuperQ requested review from SuperQ and discordianfish June 5, 2022 09:28
Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

I've merged #2378, please rebase and fix up the commit history on this change.

@rushilenekar20
Copy link
Contributor Author

rushilenekar20 commented Jun 7, 2022

Hey @SuperQ @discordianfish , I have tried a lot to rebase and fix commit history of this branch but i failed to do so after following right steps.
Now we can two things
1)u can guide me how to rebase and fix commits on this branch .
2) I have done all this changes on my new branch called rushilenekar20/diskstatsFlag
which works fine and we can merge it easily . I can create new PR with that branch

what should we do ?

This reverts commit 3624c9b.
@rushilenekar20 rushilenekar20 mentioned this pull request Jun 7, 2022
@SuperQ
Copy link
Member

SuperQ commented Jun 28, 2022

Closing in favor of #2417

@SuperQ SuperQ closed this Jun 28, 2022
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.

6 participants