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

Textfile collector: collect files from multiple paths #1262

Merged
merged 6 commits into from
Sep 21, 2023

Conversation

DiniFarb
Copy link
Contributor

Regarding #1236 and more

Added Features:

  • collect files from multiple paths
  • walkDir func for all paths to collect files from subdirs
  • new flag command collector.textfile.directories

About the collector.textfile.directories commmand:
For now it is possible to use collector.textfile.directories, collector.textfile.directory or both. They have the same functionality and in case both commands would be used, the dirs are concatenated. I am not sure if that is ok so lets discuss 😉

As soon all is settled, I'll update the docs - collector.textfile.md 😊

Copy link
Contributor

@breed808 breed808 left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this!

Is the plan to eventually deprecate the collector.textfile.directory flag in favour of the collector.textfile.directories flag? If so, it'd be worth adding a deprecation warning to the old flag (see the process collector for an example).

collector/textfile.go Outdated Show resolved Hide resolved
@DiniFarb
Copy link
Contributor Author

DiniFarb commented Aug 6, 2023

I wasn't quite sure about deprecating the collector.textfile.directory flag. Technically both flags could stay but it could lead to confusion. Maybe @JDA88 or @LaurenceChau want to give also their input about what flag would be best?

I think the best way to go is as mentioned from @breed808 - adding a deprecation warning to collector.textfile.directory flag and then remove it a few versions later.

@JDA88
Copy link
Contributor

JDA88 commented Aug 6, 2023

I think the best way to go is as mentioned from @breed808 - adding a deprecation warning to collector.textfile.directory flag and then remove it a few versions later.

I agree, as long as the current --collector.textfile.directory continue to work like today for at least a few versions

Copy link
Contributor

@breed808 breed808 left a comment

Choose a reason for hiding this comment

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

Looking good! I have one minor concern for the deprecated flag, but everything else is fine.
Thanks for updating this.

EDIT: It'd also be worth rebasing the feature branch on master to remove the merge commits.

collector/textfile.go Outdated Show resolved Hide resolved
…le path

Signed-off-by: Dinifarb <andreas.vogt89@bluewin.ch>
Signed-off-by: Dinifarb <andreas.vogt89@bluewin.ch>
Signed-off-by: Dinifarb <andreas.vogt89@bluewin.ch>
Signed-off-by: Dinifarb <andreas.vogt89@bluewin.ch>
Signed-off-by: Dinifarb <andreas.vogt89@bluewin.ch>
Signed-off-by: Dinifarb <andreas.vogt89@bluewin.ch>
@DiniFarb
Copy link
Contributor Author

I think this is ready to merge or does it need some further checks/reviews ?

Copy link
Contributor

@breed808 breed808 left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for your efforts with this one.

@breed808 breed808 merged commit 083537a into prometheus-community:master Sep 21, 2023
5 checks passed
@DiniFarb DiniFarb deleted the textfile_collector branch December 4, 2023 12:45
@DiniFarb DiniFarb mentioned this pull request Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Allow Textfile collector to collect file from multiple paths
3 participants