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 a rate limit on logger #352

Merged
merged 3 commits into from
Aug 10, 2023
Merged

Add a rate limit on logger #352

merged 3 commits into from
Aug 10, 2023

Conversation

duricanikolic
Copy link
Contributor

@duricanikolic duricanikolic commented Aug 9, 2023

What this PR does:
This PR introduces a rate limited logger RateLimitedLogger and the rate_limit_logger_discarded_log_lines_total metrics that traces the total number of discarded log lines per level

Checklist

  • Tests updated
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@duricanikolic duricanikolic force-pushed the yuri/rate-limit-log branch 2 times, most recently from 3bd7a14 to a47c7a4 Compare August 9, 2023 14:11
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Copy link
Contributor

@pracucci pracucci 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 working on this! LGTM, modulo a comment about the metrics initialization.

log/ratelimit.go Outdated Show resolved Hide resolved
log/ratelimit.go Outdated Show resolved Hide resolved
log/ratelimit_test.go Outdated Show resolved Hide resolved
log/ratelimit_test.go Show resolved Hide resolved
Signed-off-by: Yuri Nikolic <[email protected]>
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@pracucci pracucci merged commit 02eff41 into main Aug 10, 2023
@pracucci pracucci deleted the yuri/rate-limit-log branch August 10, 2023 14:29
MichelHollands added a commit to grafana/loki that referenced this pull request Aug 14, 2023
**What this PR does / why we need it**:

This PR upgrades `github.com/grafana/dskit` removes Loki's dependency on
`github.com/weaveworks/common`.

The changes in dskit, apart from the migration
(grafana/dskit#356), are:

* grafana/dskit#347 
* grafana/dskit#349, which required small
changes in `pkg/canary/reader/reader.go` and `pkg/canary/writer/push.go`
* grafana/dskit#352
* grafana/dskit#354

**Which issue(s) this PR fixes**:

(none)

**Special notes for your reviewer**:

**Checklist**
- [x] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [n/a] Documentation added
- [n/a] Tests updated
- [n/a] `CHANGELOG.md` updated
- [n/a] If the change is worth mentioning in the release notes, add
`add-to-release-notes` label
- [n/a] Changes that require user attention or interaction to upgrade
are documented in `docs/sources/setup/upgrade/_index.md`
- [n/a] For Helm chart changes bump the Helm chart version in
`production/helm/loki/Chart.yaml` and update
`production/helm/loki/CHANGELOG.md` and
`production/helm/loki/README.md`. [Example
PR](d10549e)

---------

Co-authored-by: Michel Hollands <[email protected]>
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