Skip to content
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

Merged
merged 1 commit into from
Jan 14, 2024

Conversation

ryanohnemus
Copy link
Contributor

@ryanohnemus ryanohnemus commented Dec 22, 2023


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 (via check_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 to check_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:

  • Example configuration file for the change
[SERVICE]
    flush        1
    daemon       Off
    log_level    debug
    http_server  On
    http_listen  0.0.0.0
    http_port    2020

[INPUT]
    name          kubernetes_events
    alias         k8s_events
    tag           k8s_events
  • Debug log output from testing the change
  • Attached Valgrind output that shows no leaks or memory corruption was found
    (on a mac so it was easier to use leaks)
Process:         fluent-bit [89628]
Path:            /Users/USER/*/fluent-bit
Load Address:    0x10b25e000
Identifier:      fluent-bit
Version:         0
Code Type:       X86-64
Platform:        macOS
Parent Process:  leaks [89627]

Date/Time:       2023-12-22 09:08:00.303 -0600
Launch Time:     2023-12-22 09:07:34.867 -0600
OS Version:      macOS 14.2.1 (23C71)
Report Version:  7
Analysis Tool:   /usr/bin/leaks

Physical footprint:         7444K
Physical footprint (peak):  7600K
Idle exit:                  untracked
----

leaks Report Version: 4.0, multi-line stacks
Process 89628: 596 nodes malloced for 92 KB
Process 89628: 0 leaks for 0 total leaked bytes.

[2023/12/22 09:08:00] [engine] caught signal (SIGCONT)
[2023/12/22 09:08:00] Fluent Bit Dump

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

  • Run local packaging test showing all targets (including any new ones) build.
  • Set ok-package-test label to test for all targets (requires maintainer to do).

Documentation

Backporting

  • Backport to latest stable release.

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.

@ryanohnemus ryanohnemus force-pushed the feature/k8s_events_time branch from 93ea681 to b9e0193 Compare December 22, 2023 19:52
@ryanohnemus ryanohnemus force-pushed the feature/k8s_events_time branch from bc63f9c to 377bf12 Compare January 3, 2024 13:55
@ryanohnemus ryanohnemus force-pushed the feature/k8s_events_time branch from 377bf12 to b9e0193 Compare January 3, 2024 17:04
@ryanohnemus
Copy link
Contributor Author

Test failure looks like a flake. All tests passed but the end output says “operation was canceled”

@edsiper edsiper added this to the Fluent Bit v2.2.2 milestone Jan 9, 2024
@edsiper
Copy link
Member

edsiper commented Jan 10, 2024

@ryanohnemus I think this will need some adjustment so the code does not break config compatibility even for early adopters of this plugin.

@ryanohnemus
Copy link
Contributor Author

@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']

@edsiper
Copy link
Member

edsiper commented Jan 10, 2024

@ryanohnemus I mean add them back and just ignore, then we deprecate them in v3

@ryanohnemus
Copy link
Contributor Author

@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.

@edsiper edsiper merged commit da79748 into fluent:master Jan 14, 2024
42 checks passed
@edsiper
Copy link
Member

edsiper commented Jan 14, 2024

thank you

@ryanohnemus ryanohnemus deleted the feature/k8s_events_time branch January 15, 2024 04:01
shaerpour pushed a commit to shaerpour/fluent-bit that referenced this pull request Jan 16, 2024
shaerpour pushed a commit to shaerpour/fluent-bit that referenced this pull request Jan 16, 2024
pwhelan pushed a commit that referenced this pull request Jan 16, 2024
@patrick-stephens
Copy link
Contributor

@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.

Is the option ignored now?

@ryanohnemus
Copy link
Contributor Author

yes, it's read but ignored

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants