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

Can we replace github.com/prometheus/common/log with log/slog? #121

Merged
merged 8 commits into from
Jan 9, 2024

Conversation

KacperPerschke
Copy link
Contributor

Logger github.com/prometheus/common/log is already marked as obsolete in version v0.26.0.

The log/slog component is new to the go standard library. It feels light and fast.

Hopefully we could then use it to choke out the github.com/go-kit/log library.

@peimanja peimanja self-assigned this Dec 21, 2023
@KacperPerschke
Copy link
Contributor Author

Static analysis of current master versions of imported libraries has not shown that any of these components use internally "github.com/prometheus/common/promlog".

  • 96f1aec289bfec34242831d74d450c3d0089050f "github.com/prometheus/client_golang/"
  • 9b7b6759313a7b41ffba1a65ea31cfbc9ed0b944 "github.com/prometheus/client_model/"
  • f09436866874ce72e1566783b8ee4a2c05f6bba5 "prometheus/common"

Can we continue the topic of replacing the logger with a lighter and simpler one?

I used vim and grep for analysis.

Complete replacement of `github.com/go-kit/log`
and its derivative `github.com/prometheus/common/promlog`
with `log/slog`.
@KacperPerschke
Copy link
Contributor Author

I have prepared a complete transition to log/slog. Please feel free to review and report any notices to me.

Unfortunately, I have no way to try it at home. I will try the “production” operation of my full proposal in the first week of January 2024.

@peimanja
Copy link
Owner

peimanja commented Jan 2, 2024

Thanks for your contribution. Sounds good to me. I will give this a try as well and we will sort it out soon. We can just go with the complete transition to log/slog and we'll do our testing with that

@KacperPerschke
Copy link
Contributor Author

[…] We can just go with the complete transition to log/slog […]

I moved the slog branch to this commit in my fork of your repository.

@KacperPerschke
Copy link
Contributor Author

KacperPerschke commented Jan 8, 2024

Test run results.

24 hours with logging level set to ‘debug’

We have set the ‘debug’ logging level and ‘json’ logging format.

The program behaved stably. Also when it comes to CPU and memory usage. In our environment it produced 300MB of logs per hour. At 3 a.m. service processes in the artifactory caused the program to produce over 7GB of logs and the orchestrator stopped the program.

24 hours with logging level set to ‘info’

We have set the ‘info’ logging level and ‘json’ logging format.

The program was as stable as before. In our environment, it produced less than eight log entries per day.

@peimanja
Copy link
Owner

peimanja commented Jan 8, 2024

Thanks for this change and all the testing. Looks good to me. You need to update golang version in here as well and we should be good to go.

As suggested by Peiman in his comment to PR.
Copy link

sonarcloud bot commented Jan 8, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@KacperPerschke
Copy link
Contributor Author

[…] You need to update golang version in here as well […].

Thank you for suggestion! I wasn't inquisitive enough to find it. I do hope that next commit meets the need.

Copy link
Owner

@peimanja peimanja left a comment

Choose a reason for hiding this comment

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

LGTM

@peimanja peimanja merged commit 900fd71 into peimanja:master Jan 9, 2024
2 checks passed
@KacperPerschke KacperPerschke deleted the slog branch January 9, 2024 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants