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

Add support for file-based configuration of logging #274

Merged
merged 38 commits into from
Jul 13, 2023

Conversation

venkatbvc
Copy link
Contributor

Add support for writing logs to a file, console or both.
Also support log rotation for the configured values. Changes to be committed:
modified: README.md
modified: src/logger.py
modified: src/sidecar.py

This helps end users to transform logs to the required ones by using fluentd or any other mechanism

Also support log rotation for the configured values.
Changes to be committed:
	modified:   README.md
	modified:   src/logger.py
	modified:   src/sidecar.py
@venkatbvc
Copy link
Contributor Author

@jekkel can you please review and share your comments.

chalapat and others added 5 commits May 8, 2023 13:52
Also support log rotation for the configured values.
Changes to be committed:
	modified:   README.md
	modified:   src/logger.py
	modified:   src/sidecar.py
Also support log rotation for the configured values.
Changes to be committed:
	modified:   README.md
	modified:   src/logger.py
	modified:   src/sidecar.py
Also support log rotation for the configured values.
Changes to be committed:
	modified:   README.md
	modified:   src/logger.py
	modified:   src/sidecar.py
@venkatbvc
Copy link
Contributor Author

Hi Reviewers, can you please review and provide your comments

@venkatbvc
Copy link
Contributor Author

@jekkel Can you please have a look at my Pull request?

@jekkel
Copy link
Member

jekkel commented May 31, 2023

Hi @venkatbvc

is there a specific log format you have in mind for the transformation you want to enable with this change? Just wondering whether we should provide another pre-configured format instead.... 🤔

@venkatbvc
Copy link
Contributor Author

@jekkel in our company we have a custom log format, unfortunately i cannot share the format. but it is in json format. So we have used a sidecar approach where we write logs of kiwigrid sidecar to a logfile(using klog runner :https://pkg.go.dev/k8s.io/component-base/logs/kube-log-runner) and are transforming the current format to what we need. but this solution also needs to rotate logfiles. So was thinking if we support writing to a file with log rotation then it will help people to align logs to other formats.

@venkatbvc
Copy link
Contributor Author

@jekkel We can set the log configuration from file. where user has flexibility to configure the log handlers. If this approach is fine with you , i can change and push it.

@jekkel
Copy link
Member

jekkel commented Jun 2, 2023

@jekkel We can set the log configuration from file. where user has flexibility to configure the log handlers. If this approach is fine with you , i can change and push it.

From my POV I'd rather configure the logging system than adding a transformation pipeline. In fact, there'sthe same use case for custom log formats apply for "native" cloud integration, be it GCP, AWS or whatever. For example does GCP allow for rich structured logs via JSON but for getting the most out of it one need to conform to certain conventions.

So I'm looking forward to a generic log format configuration feature 👍

@venkatbvc
Copy link
Contributor Author

@jekkel Sure i will modify my changes according the config way.

Changes to be committed:
	modified:   README.md
	modified:   src/helpers.py
	new file:   src/log_config.yaml
	modified:   src/logger.py
	modified:   src/resources.py
	modified:   src/sidecar.py
	modified:   test/resources/change_resources.yaml
	modified:   test/resources/resources.yaml
@venkatbvc
Copy link
Contributor Author

@jekkel I have made the configuration in a generic way. Please review and provide your comments.

@venkatbvc
Copy link
Contributor Author

@jekkel Did you get a chance to go through the changes?

@jekkel
Copy link
Member

jekkel commented Jun 13, 2023

Hi @tomrk-esteam8 could you please support here?

@venkatbvc
Copy link
Contributor Author

Did you a chance to look at the code?

@tomrk-esteam8
Copy link
Collaborator

It seems checks does not work. Please adjust tests.
Build and Test / Test on k8s v1.27 (pull_request) Failing after 4m

@venkatbvc
Copy link
Contributor Author

Yes I am working on it.

@venkatbvc
Copy link
Contributor Author

@tomrk-esteam8 Can you please trigger checks. I have modified code to fix the issue.

@venkatbvc
Copy link
Contributor Author

@tomrk-esteam8 All the checks have passed. Can you please check and merge if all is fine?

@venkatbvc
Copy link
Contributor Author

@tomrk-esteam8 Did you get a chance to look at the changes?

@tomrk-esteam8
Copy link
Collaborator

thanks for your contribution, could you please add some tests as well?

Copy link
Member

@jekkel jekkel left a comment

Choose a reason for hiding this comment

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

Awesome work, thanks for the contribution! From my PoV only some last bits need polishing, then this is good to go!

@tomrk-esteam8 IMO it's a minor version increment.

src/log_config.yaml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
test/resources/sidecar.yaml Show resolved Hide resolved
@venkatbvc
Copy link
Contributor Author

@jekkel I have addressed the comments. Can you trigger the build to see if the tests goes through well

Copy link
Member

@jekkel jekkel 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 you patience, a small oversight in your test is the last remaining issue from my POV.

.github/workflows/build_and_test.yaml Outdated Show resolved Hide resolved
jekkel
jekkel previously approved these changes Jul 11, 2023
@jekkel
Copy link
Member

jekkel commented Jul 11, 2023

PLease take a look at the workflow run: https://github.com/kiwigrid/k8s-sidecar/actions/runs/5520729921/jobs/10068885896?pr=274

I guess the file logging sidecar should not be waited for (unless we configure it to also log on the console?)

@venkatbvc
Copy link
Contributor Author

@tomrk-esteam8 @jekkel @ChristianGeie can you please trigger the checks.

@venkatbvc
Copy link
Contributor Author

@jekkel Yes i modified and pushed the code. can you please trigger the checks.

@ChristianGeie
Copy link
Collaborator

just triggered all checks

@venkatbvc
Copy link
Contributor Author

@ChristianGeie Thanks, i see an issue in the syntax of the issue. I fixed it. Mostly it should go through. can you please trigger checks again.

@ChristianGeie
Copy link
Collaborator

@venkatbvc sure

@venkatbvc
Copy link
Contributor Author

All, Added debug commands to identify the failure cause in the test. Can you please trigger check

@venkatbvc
Copy link
Contributor Author

@jekkel @tomrk-esteam8 @ChristianGeie Can you trigger the checks. Issue is fixed.

@venkatbvc
Copy link
Contributor Author

@jekkel All checks passed. Can you merge if the changes are OK.

@jekkel jekkel changed the title Add support for writing logs to a file, console or both. Add support for file-based configuration of logging Jul 13, 2023
@jekkel jekkel merged commit ccaf6ff into kiwigrid:master Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants