-
Notifications
You must be signed in to change notification settings - Fork 183
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
split logs + metrics Fluentd sts #588
Conversation
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 we planning to do some breaking changes in the future (especially configuration separation)?
I feel that we should refactor some configuration logic, but on the other hand that change is crucial as part of 1.0.0 which is already in pre-release phase
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.
LGTM except few changes
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.
@samjsong this is great, thank you! :)
One thing is we need to work out the details before it can be merged.
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.
Agree with other feedback and left a few minor comments. Also, we will need to make sure doc changes for any debugging depending on name are updated.
Hi team! I've made the requested changes:
|
@sumo-drosiek the CI seems to have failed due to something with the |
17b0f15
to
433db56
Compare
@samjsong I've rebased your branch on master so that building |
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.
LGTM 👍
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.
LGTM, seems to be working :)
4e24247
to
c3ed6a0
Compare
Description
Splitting Logs and Metrics Fluentd statefulsets for scalability
Also noticed that we had not updated the service monitors for Prometheus in
values.yaml
, updated those to include thefluentd
nameTesting performed