-
Notifications
You must be signed in to change notification settings - Fork 546
tail: New keep_file_handle and fstat_interval_nsec options for tail i… #2180
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
base: master
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughAdds two Tail input options ( Changes
Sequence Diagram(s)(Skipped — changes are documentation and style updates, not multi-component control flow.) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…nput #11151 Signed-off-by: David Garcia <deivid.garcia.garcia@gmail.com> Signed-off-by: deivid.garcia.garcia <deivid.garcia.garcia@gmail.com>
655a930 to
f22a8d7
Compare
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pipeline/inputs/tail.md (1)
85-95: Remove duplicate hint box on Unicode.Encoding.Lines 85-89 and 91-95 contain identical warning text about the
Unicode.Encodingparameter. Delete the redundant section to avoid reader confusion.{% endhint %} -{% hint style="info" %} -The `Unicode.Encoding` parameter is dependent on the `simdutf` library, which is itself dependent on C++ version 11 or later. In environments that use earlier versions of C++, the `Unicode.Encoding` parameter will fail. - -Additionally, the `auto` setting for `Unicode.Encoding` isn't supported in all cases, and can make mistakes when it tries to guess the correct encoding. For best results, use either the `UTF-16LE` or `UTF-16BE` setting if you know the encoding type of the target file. -{% endhint %}
🧹 Nitpick comments (1)
pipeline/inputs/tail.md (1)
479-509: Align SMB/SAMBA section wording with configuration table for consistency.Line 481 states that setting
keep_file_handletofalse"disables log rotation detection," but the configuration table (line 40) provides more nuanced guidance: "Rotation will be detected as a truncation, so some logs might be lost."Clarify that rotation detection is modified, not entirely disabled, to set proper expectations for users.
Consider updating line 481 to:
-...Note that this disables log rotation detection. +...Note that log rotation will be detected as a truncation event, which may result in some log loss.This aligns with the more detailed explanation in the configuration table and provides users with a clearer understanding of the tradeoff.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pipeline/inputs/tail.md(2 hunks)
🔇 Additional comments (2)
pipeline/inputs/tail.md (2)
40-41: Configuration parameters clearly documented with appropriate defaults.The new
keep_file_handleandfstat_interval_nsecparameters are well-described with clear guidance on tradeoffs (e.g., rotation detection implications, IOPS sensitivity). The placement in the table is appropriate, and defaults are specified.
511-595: Character Encoding Conversion section is well-structured and provides excellent guidance.The expanded section clearly explains when and how to use encoding conversion, with good organization of encoding types and practical examples. The separation between
Unicode.EncodingandGeneric.Encodinguse cases is helpful.
alexakreizinger
left a comment
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.
thanks for submitting a PR :)
I added a few suggestions based on our style guide, but let me know if I got anything wrong. (also, I believe that "Samba" should be a proper noun and not an acronym, but if that's not correct I can fix it!)
Co-authored-by: Alexa Kreizinger <alexakreizinger@gmail.com> Signed-off-by: David <deivid.garcia.garcia@gmail.com>
Co-authored-by: Alexa Kreizinger <alexakreizinger@gmail.com> Signed-off-by: David <deivid.garcia.garcia@gmail.com>
Co-authored-by: Alexa Kreizinger <alexakreizinger@gmail.com> Signed-off-by: David <deivid.garcia.garcia@gmail.com>
|
@david-garcia-garcia failing on some lint issues... if you can take a look so this is ready when the code PR merges? |
Signed-off-by: Alexa Kreizinger <alexakreizinger@gmail.com>
|
@eschabell I just caught/fixed the failing check, should be able to merge now :) |
# Conflicts: # pipeline/inputs/tail.md
|
@david-garcia-garcia linting errors need to be addressed before review please. |
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@vale-styles/FluentBit/Headings.yml`:
- Around line 145-148: The list item "Tailing files from SMB/Samba" is misplaced
in the S-section; remove the "Tailing files from SMB/Samba" entry between "SQL"
and "SSL" and insert it into the T-section (for example, directly after "TCP")
so the overall list remains alphabetically ordered; ensure you update only the
list ordering and preserve the exact text "Tailing files from SMB/Samba".
| - SMB | ||
| - SQL | ||
| - Tailing files from SMB/Samba | ||
| - SSL |
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.
Tailing files from SMB/Samba is out of alphabetical order.
SMB on line 145 is correctly placed. However, Tailing files from SMB/Samba (starting with "T") is inserted between SQL and SSL, breaking the alphabetical ordering of the list. It should be moved to the T-section, e.g., after TCP (line 154).
Proposed fix
- SMB
- SQL
- - Tailing files from SMB/Samba
- SSL
- SSM
- StatsD
- Studio
- Sumo Logic
- Tanzu
- TCP
+ - Tailing files from SMB/Samba
- Telemetry Pipeline🤖 Prompt for AI Agents
In `@vale-styles/FluentBit/Headings.yml` around lines 145 - 148, The list item
"Tailing files from SMB/Samba" is misplaced in the S-section; remove the
"Tailing files from SMB/Samba" entry between "SQL" and "SSL" and insert it into
the T-section (for example, directly after "TCP") so the overall list remains
alphabetically ordered; ensure you update only the list ordering and preserve
the exact text "Tailing files from SMB/Samba".
|
@eschabell I have run vale locally to fix the issues, but I am getting unexpected results about errors with something not modified in this PR. Can the workflow be triggered again? Thanks. |
See fluent/fluent-bit#11151
Summary by CodeRabbit