Add a flag to adjust mount timeout

[mknapphrt]

Allow the user to specify a timeout for considering mounts stale. Previously this was set to a static 30s fairly arbitrarily. I lowered the default because I think it should be lower than the default scrape rate for prometheus so that a timeout doesn't cause a failed scrape. The reason I feel this should be adjustable is that it could be useful to change this value to better match a users scrape rate and the known load on a filesystem.

A couple scenarios that come to mind would be if an nfs mount goes stale and someone has a low scrape rate, say 5-10s, this means that there will be 3-6 failed scrapes from timing out. The user would probably rather set this timeout lower. Or the opposite, if a filesystem is known to be under heavy load at times, a user may not want to timeout quickly.

Signed-off-by: Mark Knapp mknapp@hudson-trading.com

[mknapphrt]

@SuperQ @pgier @discordianfish Any chance this could get a look?

[SuperQ]

I don't think the 30s was completely arbitrary. Some of the idea was that it was a reasonable value that was a multiple of the scrape interval.

I could see changing the value from 30s to 10s, but making it a flag seems unecessary. It also may cause users to unnecessarily tweak it without fully understanding the implications.

[mknapphrt]

If the idea was to be a multiple of the scrape interval, and the user is able to select their desired scrape interval, shouldn't they be able to update something like this then to coincide? Raising or lowering the scrape interval usually involves updating timeouts in other places too. I know there isn't a direct connection between scrape rate and mount timeouts, but the number of missed scrapes on a timeout is based on a function of these two values.

If the concern is that there isn't enough understanding in what the flag will change, I can give it a more verbose description. But I think "because the user might change it without understanding it" isn't a great reason not to make it an option to anyone. It's on the user at that point to not modify settings they don't understand.

[SuperQ]

I know you think it's not a great reason, and you're probably right, but with these kinds of tuneables we see far too many support issues already with the existing amount of flexibility. Every tuning dimension we add expands the support matrix a little bit more.

I would prefer to dig into the history of the current default and find out why the number was chosen, rather than add another -funroll-loops tuning flag.

[mknapphrt]

It basically just boiled down to #997 (comment)

Not sure, maybe 30s?

There wasn't much of a discussion about it at the time, kinda just went with it. The question of a flag was dismissed at the time because I didn't see a need for it then, but i'm finding now that 30s is longer than necessary and causing multiple missed scrapes for us.

[SuperQ]

@discordianfish Do you have any opinion here?

[discordianfish]

I'd be fine merging this but don't feel strongly. @pgier for tie breaking? :)

[pgier]

I'm ok with merging. I think @mknapphrt's reasoning makes sense, and I'd probably lower the timeout even more (5s or less) since monitoring fliesystems with such a slow response should be fairly uncommon (I hope).

[mknapphrt]

I reduced the time a little more, I think 5s is reasonable too. What do you think @SuperQ ?

[SuperQ]

Seems fine to me. I still think I want to limit end-user exposure to this tunable. As a compromise, what do you think about making this a .Hidden() flag? This way it's available to users who are interested enough to read the source, but isn't encouraged for random users?

[mknapphrt]

Yeah, that sounds fair enough to me. I don't really know how many people would care about this functionality so keeping it hidden is fine.

[SuperQ]

LGTM, Thanks!

[mknapphrt]

Any idea when we might see another release cut?

[SuperQ]

There are a couple of moderate breaking changes we want to get in before we do a release. The next release we want to do will be a 1.0 release candidate.