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: make timestamp from incoming record configurable #8289

Merged
merged 2 commits into from
Dec 22, 2023

Conversation

ryanohnemus
Copy link
Contributor

@ryanohnemus ryanohnemus commented Dec 15, 2023

input kubernetes_events currently uses a record accessor that is hard coded to $metadata['creationTimestamp']. This patch allows it to be configurable so that you can use other timestamp fields as the record timestamp such as lastTimestamp which will have the most recent event's timestamp and not the time the of the first matching kubernetes event (which could be hours/days prior) which is what the $metadata['creationTimestamp'] is set to.


Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

  • [ X] Example configuration file for the change
[INPUT]
    Name kubernetes_events
    Tag k8s_events
    timestamp_key lastTimestamp
  • Debug log output from testing the change

  • Attached Valgrind output that shows no leaks or memory corruption was found

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

  • Documentation required for this feature

Docs PR: fluent/fluent-bit-docs#1274

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.

@edsiper
Copy link
Member

edsiper commented Dec 18, 2023

@ryanohnemus thanks for this PR! adding @pwhelan in the loop here.

This PR looks good to me, but before the merge I wanted to ask you both if we should consider to move the default to lastTimestamp ?

@ryanohnemus
Copy link
Contributor Author

@edsiper i think it would make sense as the default, by definition:

lastTimestamp - The time at which the most recent occurrence of this event was recorded.

Would you like me to push a new commit with the default change?

@edsiper
Copy link
Member

edsiper commented Dec 21, 2023

@ryanohnemus thanks, yes please, ASAP so we can make the cut today

@ryanohnemus
Copy link
Contributor Author

@edsiper done! i also updated the documentation PR just now as well: fluent/fluent-bit-docs#1274 so that might need re-approval/review

@ryanohnemus
Copy link
Contributor Author

actually, i think my last push isn't right... give me a min to fix...

@ryanohnemus ryanohnemus force-pushed the feature/input_kubernetes_events branch from 9445b87 to 95c6afe Compare December 21, 2023 23:10
@ryanohnemus
Copy link
Contributor Author

@edsiper ok, sorry about that, please take another look, we should be good to go. I had a work in progress commit that i accidentally had pushed after your original review.

@edsiper
Copy link
Member

edsiper commented Dec 21, 2023

great thanks, waiting for CI

@@ -38,7 +38,7 @@
#define K8S_EVENTS_KUBE_TOKEN "/var/run/secrets/kubernetes.io/serviceaccount/token"
#define K8S_EVENTS_KUBE_CA "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt"

#define K8S_EVENTS_RA_TIMESTAMP "$metadata['creationTimestamp']"
#define K8S_EVENTS_RA_TIMESTAMP "lastTimestamp"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this just using that text instead of a record accessor pattern ?

Copy link
Contributor Author

@ryanohnemus ryanohnemus Dec 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to work with either lastTimestamp or $lastTimestamp as lastTimestamp is a top-level field.

If it were not able to find the data i'd expect to see this error in the log:

rval = flb_ra_get_value_object(ctx->ra_timestamp, *item);
if (!rval || rval->type != FLB_RA_STRING) {
flb_plg_error(ctx->ins, "cannot retrieve event timestamp");
goto msg_error;
}

Using a stdout output also looks correct to me:

[40] k8s_events: [[1703206505.000000000, {}], {"metadata"=>{"name"=>"storage-provisioner.17a29100c848d8b6", "namespace"=>"kube-system", "uid"=>"dd4006f8-2916-42fb-bc92-dc0a424978a3", "resourceVersion"=>"130490", "creationTimestamp"=>"2023-12-22T00:55:05Z", "managedFields"=>[{"manager"=>"kubelet", "operation"=>"Update", "apiVersion"=>"v1", "time"=>"2023-12-22T00:55:05Z", "fieldsType"=>"FieldsV1", "fieldsV1"=>{"f:count"=>{}, "f:firstTimestamp"=>{}, "f:involvedObject"=>{}, "f:lastTimestamp"=>{}, "f:message"=>{}, "f:reason"=>{}, "f:reportingComponent"=>{}, "f:reportingInstance"=>{}, "f:source"=>{"f:component"=>{}, "f:host"=>{}}, "f:type"=>{}}}]}, "involvedObject"=>{"kind"=>"Pod", "namespace"=>"kube-system", "name"=>"storage-provisioner", "uid"=>"a5f87337-043b-47b5-8a9c-0c603c15a01e", "apiVersion"=>"v1", "resourceVersion"=>"84639", "fieldPath"=>"spec.containers{storage-provisioner}"}, "reason"=>"BackOff", "message"=>"Back-off restarting failed container storage-provisioner in pod storage-provisioner_kube-system(a5f87337-043b-47b5-8a9c-0c603c15a01e)", "source"=>{"component"=>"kubelet", "host"=>"minikube"}, "firstTimestamp"=>"2023-12-20T14:37:21Z", "lastTimestamp"=>"2023-12-22T00:55:05Z", "count"=>15, "type"=>"Warning", "eventTime"=>nil, "reportingComponent"=>"kubelet", "reportingInstance"=>"minikube"}]

The documentation mentions we should use a prefixed $ for record accessors so if you'd like me to update this I can push another commit, just let me know!

Copy link
Contributor Author

@ryanohnemus ryanohnemus Dec 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edsiper Actually, I just went ahead and updated this as it would bug me if I wasn't following the documentation :). I also updated the docs pr at the same time: fluent/fluent-bit-docs#1274

@ryanohnemus ryanohnemus force-pushed the feature/input_kubernetes_events branch from 17c2ca2 to 765141b Compare December 22, 2023 01:19
@edsiper
Copy link
Member

edsiper commented Dec 22, 2023

it seems CI is stuck

@ryanohnemus
Copy link
Contributor Author

Is there anything I can do to fix it?

@ryanohnemus
Copy link
Contributor Author

ryanohnemus commented Dec 22, 2023

I think when I force pushed it took away the approval. Does it need that for the other checks to run/report?

@edsiper edsiper merged commit 78e79b7 into fluent:master Dec 22, 2023
6 checks passed
@edsiper
Copy link
Member

edsiper commented Dec 22, 2023

all good, since the past one passed no need for another one., thank you

@ryanohnemus ryanohnemus deleted the feature/input_kubernetes_events branch December 22, 2023 13:04
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.

2 participants