-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
enhancement(kubernetes_logs source): Wire up acknowledgement support via existing file ack support #22366
base: master
Are you sure you want to change the base?
Conversation
Hi @ganelo and thank you for this PR. The |
6cb511b
to
ed6c692
Compare
Can you share the failure? The script requires Ruby 3, which often trips people up. |
I verified that it is not broken master.
This PR adds configuration fields and we rely on source code to generate docs. Hence, docs need to regenerated. As @jszwedko pointed out we have a dependency on Ruby 3. My version locally:
|
Ah okay, thanks for clarifying! Will try again tomorrow with the correct
version of Ruby to get to the bottom of the failure.
…On Thu, Feb 6, 2025, 18:37 Pavlos Rontidis ***@***.***> wrote:
Hi @pront <https://github.com/pront> - this seems to be broken on master,
as far as I can tell.
I verified that it is not broken master.
In any case, nothing I've touched should have broken the ruby script. Not
sure how best to proceed.
This PR adds configuration fields and we rely on source code to generate
docs. Hence, docs need to regenerated. As @jszwedko
<https://github.com/jszwedko> pointed out we have a dependency on Ruby 3.
My version locally:
❯ ruby -v
ruby 3.1.4p223 (2023-03-30 revision 957bb7cb81) [arm64-darwin22]
—
Reply to this email directly, view it on GitHub
<#22366 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB4OKWXU3ELUEEVUJQVLFDD2OPW3BAVCNFSM6AAAAABWPOJTRWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNBRGM2TKNRZGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Looks better now 👍 Is it possible to add a test to cover the acks feature? You can draw inspiration from other sources e.g. the Lines 2375 to 2376 in b90715b
|
To confirm - there aren't any existing tests that actually exercise the kubernetes_logs source, right? I poked around and didn't find much that did anything beyond some niche unit tests. Certainly nothing to mirror the I'm not utterly opposed to writing one from more or less whole cloth, but it will be somewhat slow going as I'm a relative newcomer to Rust. |
Summary
Mirror changes made to file source as part of implementing acknowledgements support there over to kubernetes_logs source.
Change Type
Is this a breaking change?
How did you test this PR?
Given that the underlying feature work for implementing acknowledgements in the file source is well-tested, my plan is to deploy a custom build of vector with this change to a dev environment and simply smoke test to confirm that, when acknowledgements are enabled via config, we see evidence that log events are not considered processed until they are fully delivered at the end of the sink chain (eg by counting unique log event ids at the end of the test after bouncing some of the intermediate hops in chain during the test).
Does this PR include user facing changes?
Checklist
make check-all
is a good command to run locally. This check isdefined here. Some of these
checks might not be relevant to your PR. For Rust changes, at the very least you should run:
cargo fmt --all
cargo clippy --workspace --all-targets -- -D warnings
cargo nextest run --workspace
(alternatively, you can runcargo test --all
)Cargo.lock
), pleaserun
dd-rust-license-tool write
to regenerate the license inventory and commit the changes (if any). More details here.References
kubernetes_logs
source #7462file
source #7459 for ACK support in file source, which this PR depends on (implemented in enhancement(file source): Add support for acknowledgements #7816)