Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[new feature] promtail: Add config reload endoint / signal to promtail #7247
[new feature] promtail: Add config reload endoint / signal to promtail #7247
Changes from 24 commits
0c7e77c
f7d7244
b9c89d5
0b7e536
07a00bd
9e82c9f
4bdf6cd
ce7ddb0
e01b8bd
4d6ee77
69489b9
646b120
ebc0e35
df6f15d
f838787
3074a1e
9bd469f
ab2dd37
6bc47e3
6651e62
2817f60
a2809cd
b980753
41b584f
5e2dd98
489f9cf
5582e95
dba1290
3928fff
3396a6f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's please use the stdlib errors where possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please wrap this error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think let's move this into the condition on L107 so that it only logs when a change is made.
Alternatively, you could change this to a
debug
level and add aninfo
level log line in the condition on L107There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This log message won't mean much to the user. Can we return the actual parse error here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, not super familiar with these live reload patterns, can you explain the SIGHUP flow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refer to the reload documentation of prometheus, we should also provide this reload way.
https://prometheus.io/docs/prometheus/latest/configuration/configuration/
A configuration reload is triggered by sending a SIGHUP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use
fmt.Errorf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these
200
and500
codes meant to mimick HTTP status codes?If so, I think that's confusing. Let's be clear here and have two separate metrics - one for successful reloads and one for failures.