-
Notifications
You must be signed in to change notification settings - Fork 1.8k
router: extend router interface to support metrics (v3) #11108
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
Merged
Merged
Changes from 16 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
c76f9ad
router: add router context structure and move routing state
edsiper d4ad577
router: implement router context creation and lifecycle
edsiper e9d4ae3
routes_mask: migrate API to use router context instead of config
edsiper 4094f8d
config: integrate router context creation and destruction
edsiper a56488e
engine: publish routing metrics on log batch flush and drop
edsiper 953d22b
input_chunk: update routing mask operations to use router context
edsiper 046e0dc
input_log: update routing mask operations to use router context
edsiper 8b50dd3
metrics_exporter: merge router metrics into /metrics output
edsiper a1d1bbb
in_storage_backlog: update routing mask operations to use router context
edsiper 7eba8fc
task: update routing mask checks to use router context
edsiper aad9038
tests: internal: routing: update routing tests to use router context
edsiper e934e60
input_chunk: small cleanup
edsiper 9e176f4
router: remove unnecessary function
edsiper a31157e
tests: internal: input_chunk_routes: fix compiler warnings
edsiper 781aedf
tests: internal: fuzzers: aws_credentials_fuzzer: fix incorrect lengt…
edsiper 5c65379
config: release kernel context on router creation error
edsiper f15ede8
bin: destroy cf opts on failure
edsiper 3ee2305
config: release http resources on exit
edsiper File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
Over-allocation: using slots (bits) instead of size (elements) for mask operations.
flb_routes_mask_get_slots() returns bit-count; you’re allocating/clearing 64× more than needed. Use flb_routes_mask_get_size() for element count.
Apply this fix:
📝 Committable suggestion
🤖 Prompt for AI Agents
Uh oh!
There was an error while loading. Please reload this page.
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.
Yeah, it could be an optimized way. But I have a question. If we use this way to allocate heap memories, it'll need to reallocate to fix the size of heap memories for route masks? This is because it could be using a possibly bigger size of allocation here, but the re-allocate costs could be high or any other re-allocating places are implemented to extend/shrink the size of heap memories that are using for route masks?
If there's no code path to re-allocate/adjust places for them, it could be worthless suggestion.
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.
🧩 Analysis chain
🏁 Script executed:
Length of output: 84
🏁 Script executed:
Length of output: 444
🏁 Script executed:
Length of output: 316
🏁 Script executed:
Length of output: 2028
🏁 Script executed:
Length of output: 856
@cosmo0920, great question! Let me clarify - there's no reallocation happening here, and this is actually a bug fix, not just an optimization.
The Issue
The semantic difference between the two functions:
route_mask_slots= number of bits (i.e., max number of outputs/routes)route_mask_size= number of elements (uint64_t) =ceil(slots / 64)Since each
flb_route_mask_element(uint64_t) holds 64 bits, if you have 3 outputs, you need:route_mask_slots= 3 (bits)route_mask_size= 1 (element, since 3 bits fit in one uint64_t)Current Bug
The storage_backlog code currently:
slotselements (3 elements = 192 bytes) when it only needssizeelements (1 element = 8 bytes)slotselements in the loop when it only needs to clearsizeelementsThis is a ~64× over-allocation and over-clearing, not a design choice.
No Reallocation Occurs
The lifecycle is simple:
cb_sb_init(line 671)sb_append_chunk_to_segregated_backlogs(line 289) — called in a loopcb_sb_exitThe mask size is determined at router initialization and never changes. There's no code path that reallocates or adjusts this buffer.
Consistency with Rest of Codebase
Every other allocation/clear operation in the refactored code uses
get_size():src/flb_input_chunk.cline 964 & 2024: allocations useget_size()src/flb_input_log.cline 182: memset usesget_size()src/flb_routes_mask.cline 67: memset usesget_size()The storage_backlog plugin is the only place still using
get_slots()incorrectly.Recommendation
Apply the fix to match the rest of the codebase. It will:
🧬 Code Graph Analysis Results
[src/flb_routes_mask.c] lines 36-43
[src/flb_routes_mask.c] lines 49-88
[src/flb_routes_mask.c] lines 154-173
[src/include/fluent-bit/flb_mem.h] lines 84-96
[src/include/fluent-bit/flb_mem.h] lines 126-128
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.
@edsiper This @coderabbitai's point of view could be correct for this patch. So, could you take a look on it?
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.
@cosmo0920, thank you for the review! I appreciate you confirming the analysis.
@edsiper, the issue is that both the allocation (line 671) and memset (line 289) in the storage_backlog plugin are using
flb_routes_mask_get_slots()where they should useflb_routes_mask_get_size(). This causes a 64× over-allocation sinceslotscounts bits whilesizecounts uint64_t elements.The fix is straightforward and makes the code consistent with the rest of the router refactoring. Happy to clarify anything if needed!