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

Load rule files from directory or glob. #741

Closed
beorn7 opened this Issue May 27, 2015 · 15 comments

Comments

Projects
None yet
4 participants
@beorn7
Copy link
Member

beorn7 commented May 27, 2015

In the config, rules files have to be specified explicitly at the moment. It should be possible to either specify a glob or a directory. Upon startup and upon a SIGHUP, all files matching the specified glob or all files matching *.rules in the specified directory, respectively, will be loaded.

Note that the latter corresponds to the way the textfile module of the node exporter works.

Both ways allows applying atomic renaming to avoid parsing of incomplete files (i.e. we do not want an implementation that simply loads all files in a specified directory).

@beorn7

This comment has been minimized.

Copy link
Member Author

beorn7 commented May 27, 2015

BTW: I prefer the <directory>/*.rules way over the glob way.

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented May 27, 2015

Me too – it's also more consistent with the file_sd_configs.

@beorn7

This comment has been minimized.

Copy link
Member Author

beorn7 commented May 28, 2015

You actually mean the opposite. ;)

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented May 28, 2015

See my comment in the PR. Glob is simply used to expand the files in the directory (plus allowing prefixes and suffixes on file level). Globing directories is not possible through the config.

@beorn7

This comment has been minimized.

Copy link
Member Author

beorn7 commented May 28, 2015

Yes.

But my idea was, like in the textfile exporter, to not allow globs at all, but only either a complete file name or a directory. In the former case, everything stays as it is (so no breaking change). In the latter case, only files with a hardcoded suffix (e.g. .rules) are loaded from that directory.

I like my idea best (hear hear...), but I prefer consistency within the same binary. So we should go with exactly the same approach as the file-based SD.

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented May 28, 2015

I think the current solution is closer to your idea than to full-fledged globing – at least I hope so :)

We could of course enforce .rules ending as well if no one feels strong about potentially breaking people who chose another extension for some reason.

@beorn7

This comment has been minimized.

Copy link
Member Author

beorn7 commented May 28, 2015

I think the current solution is closer to your idea than to full-fledged globing – at least I hope so :)

But perhaps globbing is not desirable per se.

Also, we don't need to enforce .rules as a prefix in general. According to my idea, that would only required for files in configured directories.

But as said, unless we now want to change file-based SD, too, we should just stay with the approach you have chosen.

@matthiasr

This comment has been minimized.

Copy link
Contributor

matthiasr commented May 28, 2015

FWIW, taking everything from a directory is not incompatible with atomic renames either – as long as it's on the same filesystem, whether the mv is within a directory or not doesn't matter.

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented May 28, 2015

Yes, if you have a set of fixed file extensions that are considered only,
this is not an issue.

On Thu, May 28, 2015 at 12:56 PM Matthias Rampke notifications@github.com
wrote:

FWIW, taking everything from a directory is not incompatible with atomic
renames either – as long as it's on the same filesystem, whether the mv
is within a directory or not doesn't matter.


Reply to this email directly or view it on GitHub
#741 (comment)
.

@beorn7

This comment has been minimized.

Copy link
Member Author

beorn7 commented May 28, 2015

On Thu, May 28, 2015 at 12:56 PM, Matthias Rampke
notifications@github.com wrote:

FWIW, taking everything from a directory is not incompatible with atomic renames either – as long as it's on the same filesystem, whether the mv is within a directory or not doesn't matter.

Sure. Technically, it's not a problem, but we are expecting human
beings to do it the right way...

Björn Rabenstein, Engineer
http://soundcloud.com/brabenstein

SoundCloud Ltd. | Rheinsberger Str. 76/77, 10115 Berlin, Germany
Managing Director: Alexander Ljung | Incorporated in England & Wales
with Company No. 6343600 | Local Branch Office | AG Charlottenburg |
HRB 110657B

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented May 28, 2015

Renaming being atomic is not a general assumption we can make anyway,
unfortunately.

On Thu, May 28, 2015 at 1:17 PM Björn Rabenstein notifications@github.com
wrote:

On Thu, May 28, 2015 at 12:56 PM, Matthias Rampke
notifications@github.com wrote:

FWIW, taking everything from a directory is not incompatible with atomic
renames either – as long as it's on the same filesystem, whether the mv is
within a directory or not doesn't matter.

Sure. Technically, it's not a problem, but we are expecting human
beings to do it the right way...

Björn Rabenstein, Engineer
http://soundcloud.com/brabenstein

SoundCloud Ltd. | Rheinsberger Str. 76/77, 10115 Berlin, Germany
Managing Director: Alexander Ljung | Incorporated in England & Wales
with Company No. 6343600 | Local Branch Office | AG Charlottenburg |
HRB 110657B


Reply to this email directly or view it on GitHub
#741 (comment)
.

@fabxc fabxc closed this Jun 1, 2015

@davidkarlsen

This comment has been minimized.

Copy link

davidkarlsen commented Oct 7, 2016

The issue is closed - but was this ever implemented?

@beorn7

This comment has been minimized.

Copy link
Member Author

beorn7 commented Oct 7, 2016

Yes, it's globbing.

fs, err := filepath.Glob(pat)

We should document the behavior.

@beorn7

This comment has been minimized.

Copy link
Member Author

beorn7 commented Oct 7, 2016

simonpasquier pushed a commit to simonpasquier/prometheus that referenced this issue Oct 12, 2017

Add Tincho to the list of consultants (prometheus#741)
I was told that it was OK to directly sending a PR for this. Please, let me know if it is not.
@lock

This comment has been minimized.

Copy link

lock bot commented Mar 24, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 24, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.