-
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: make timestamp from incoming record configurable #8289
in_kubernetes_events: make timestamp from incoming record configurable #8289
Conversation
Signed-off-by: ryanohnemus <[email protected]>
22b93d6
to
047ca27
Compare
@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 ? |
@edsiper i think it would make sense as the default, by definition:
Would you like me to push a new commit with the default change? |
@ryanohnemus thanks, yes please, ASAP so we can make the cut today |
@edsiper done! i also updated the documentation PR just now as well: fluent/fluent-bit-docs#1274 so that might need re-approval/review |
actually, i think my last push isn't right... give me a min to fix... |
9445b87
to
95c6afe
Compare
@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. |
95c6afe
to
17c2ca2
Compare
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" |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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:
fluent-bit/plugins/in_kubernetes_events/kubernetes_events.c
Lines 512 to 516 in 17c2ca2
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!
There was a problem hiding this comment.
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
Signed-off-by: ryanohnemus <[email protected]>
17c2ca2
to
765141b
Compare
it seems CI is stuck |
Is there anything I can do to fix it? |
I think when I force pushed it took away the approval. Does it need that for the other checks to run/report? |
all good, since the past one passed no need for another one., thank you |
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 aslastTimestamp
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:
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.
ok-package-test
label to test for all targets (requires maintainer to do).Documentation
Docs PR: fluent/fluent-bit-docs#1274
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.