8000 feat(dev): Enable `internal_log_rate_limit` by default by shivanthzen · Pull Request #22899 · vectordotdev/vector · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat(dev): Enable internal_log_rate_limit by default #22899

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.

Sign up 8000 for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat(dev): Enable internal_log_rate_limit by default #22899

wants to merge 4 commits into from

Conversation

shivanthzen
Copy link
@shivanthzen shivanthzen commented Apr 17, 2025 •
edited
Loading

Summary

Fixes: #22410

Change Type

  • Bug fix
  • New feature
  • Non-functional (chore, refactoring, docs)
  • Performance

Is this a breaking change?

  • Yes
  • No

How did you test this PR?

Unit tests

Does this PR include user facing changes?

  • Yes. Please add a changelog fragment based on our guidelines.
  • No. A maintainer will apply the "no-changelog" label to this PR.

Notes

  • Please read our Vector contributor resources.
  • Do not hesitate to use @vectordotdev/vector to reach out to us regarding this PR.
  • The CI checks run only after we manually approve them.
    • We recommend adding a pre-push hook, please see this template.
    • Alternatively, we recommend running the following locally before pushing to the remote branch:
      • cargo fmt --all
      • cargo clippy --workspace --all-targets -- -D warnings
      • cargo nextest run --workspace (alternatively, you can run cargo test --all)
      • ./scripts/check_changelog_fragments.sh
  • After a review is requested, please avoid force pushes to help us review incrementally.
    • Feel free to push as many commits as you want. They will be squashed into one before merging.
    • For example, you can run git merge origin master and git push.
  • If this PR introduces changes Vector dependencies (modifies Cargo.lock), please
    run cargo vdev build licenses to regenerate the license inventory and commit the changes (if any). More details here.

References

@bits-bot
Copy link
bits-bot commented Apr 17, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added domain: sources Anything related to the Vector's sources domain: transforms Anything related to Vector's transform components domain: sinks Anything related to the Vector's sinks labels Apr 17, 2025
shivanthzen
shivanthzen commented Apr 17, 2025
View reviewed changes
lib/tracing-limit/src/lib.rs
8000
@@ -129,7 +129,7 @@ where
let mut limit_visitor = LimitVisitor::default();
event.record(&mut limit_visitor);

let limit_exists = limit_visitor.limit.unwrap_or(false);
let limit_exists = limit_visitor.limit.unwrap_or(true);
Copy link
Author
@shivanthzen shivanthzen Apr 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is where the default is changed

@shivanthzen shivanthzen changed the title feat: Enablet internal ratelimitng by default feat: Enable internal ratelimitng by default Apr 17, 2025
@shivanthzen shivanthzen marked this pull request as ready for review April 24, 2025 20:39
@shivanthzen shivanthzen requested a review from a team as a code owner April 24, 2025 20:39
@shivanthzen
Copy link
Author

@pront

@pront pront changed the title feat: Enable internal ratelimitng by default feat(dev): Enable internal_log_rate_limit by default Apr 28, 2025
pront
pront approved these changes Apr 28, 2025
View reviewed changes
Copy link
Member
@pront pront left a comment

Choose a reason for hiding this comment

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

This looks good, thank you @shivanthzen!

@pront pront added the no-changelog Changes in this PR do not need user-facing explanations in the release changelog label Apr 28, 2025
@pront
Copy link
Member
pront commented Apr 28, 2025

Let's also add a small section here: https://github.com/vectordotdev/vector/blob/master/src/cli.rs#L180

/// Set the internal log rate limit. 
/// Note that traces are throttled by default unless tagged with `internal_log_rate_limit = false`.

@pront pront enabled auto-merge April 29, 2025 18:14
@shivanthzen
Copy link
Author

Hmm, some of the tests failed because the rate limit value is not defined 🤔 in those cases. Let me check that.

@pront
Copy link
Member
pront commented May 9, 2025

Hmm, some of the tests failed because the rate limit value is not defined 🤔 in those cases. Let me check that.

Thank you. Feel free to ping me if you need anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pront pront pront approved these changes

Assignees
No one assigned
Labels
domain: sinks Anything related to the Vector's sinks domain: sources Anything related to the Vector's sources domain: transforms Anything related to Vector's transform components no-changelog Changes in this PR do not need user-facing explanations in the release changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

internal_log_rate_limit should be ON by default
3 participants
@shivanthzen @bits-bot @pront
0