in_kubernetes_events: consolidate record timestamp logic#8323
in_kubernetes_events: consolidate record timestamp logic#8323edsiper merged 1 commit intofluent:masterfrom
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 <ryanohnemus@gmail.com>
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 <ryanohnemus@gmail.com>
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_filteredinstead 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-testlabel 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.