-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
in_kubernetes_events: consolidate record timestamp logic #8323
Conversation
93ea681
to
b9e0193
Compare
bc63f9c
to
377bf12
Compare
377bf12
to
b9e0193
Compare
Test failure looks like a flake. All tests passed but the end output says “operation was canceled” |
@ryanohnemus I think this will need some adjustment so the code does not break config compatibility even for early adopters of this plugin. |
@edsiper ok! What do you think would be the best route? I could add the timestamp_key config back in but just ignore it when it's read... or I could add the timestamp_key back in and if it is set attempt to use that field for the record timestamp if it's present, if not present I could fallback to searching through the rest of the fields in order lastTimestamp, firstTimestamp, $metadata['creationTimestamp'] |
@ryanohnemus I mean add them back and just ignore, then we deprecate them in v3 |
Signed-off-by: ryanohnemus <[email protected]>
b9e0193
to
d192c65
Compare
@edsiper I added that config option back in with a note we'll remove it in v3.0. I also rebased off of master at the same time to ensure there weren't any conflicts. |
thank you |
Signed-off-by: ryanohnemus <[email protected]>
Signed-off-by: ryanohnemus <[email protected]> Signed-off-by: ahspw <[email protected]>
Signed-off-by: ryanohnemus <[email protected]>
Is the option ignored now? |
yes, it's read but ignored |
This consolidates timestamp lookup logic in the kubernetes_events input and removes record accessor timestamp key logic entirely as it's not needed.
Currently during events processing logic we are fetching the timestamp from the record twice in 2 different ways.
We first use
item_get_timestamp()
that cleanly fetches the timestamp from any of the following record values in order of precedence: lastTimestamp, firstTimestamp, metadata.creationTimestamp, this is then used to look up if the current event is older than the set retention time and if so the event is discarded (viacheck_event_is_filtered
). We then immediately fetch the timestamp again to set it in the event, but only look at a single field from the resource accessor (K8S_EVENTS_RA_TIMESTAMP
).This diff removes the record accessor, and it's config, because the safest option is to just check in order of preference for a proper timestamp which
item_get_timestamp()
already does. It also fetches the timestamp from the incoming record only once, then just passes the time tocheck_event_is_filtered
instead of having 2 different ways of finding the event time.Testing
Before we can approve your change; please submit the following in a comment:
(on a mac so it was easier to use leaks)
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-test
label to test for all targets (requires maintainer to do).Documentation
in_kubernetes_events: update to match consolidated timestamp logic fluent-bit-docs#1277
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.