-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[receiver/k8sobjects] ensure the k8s.namespace.name
attribute is set for objects retrieved using the watch
mode
#36432
base: main
Are you sure you want to change the base?
[receiver/k8sobjects] ensure the k8s.namespace.name
attribute is set for objects retrieved using the watch
mode
#36432
Conversation
… and use same data structure as with pull based mode for log entries Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
pull
and watch
mode
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
@@ -37,6 +34,7 @@ func watchObjectsToLogData(event *watch.Event, observedAt time.Time, config *K8s | |||
if name != "" { | |||
attrs.PutStr("event.domain", "k8s") | |||
attrs.PutStr("event.name", name) | |||
attrs.PutStr("event.type", string(event.Type)) |
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.
FYI: there is an issue regarding reflecting of k8s events to the events API definition - #35857 (comment), maybe it makes sense to include event.type
to the events api convention
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.
Thank's @bacherfl for working on this!
I agree that both pull
and watch
modes should populate the k8s.namespace.name
resource attribute as it was highlighted at #36352.
However, I'm not sure if the existing structure should change, object.metadata.*
-> metadata.*
.
Isn't this a breaking change? Should we ensure if we really want this or not?
Otherwise, if we can fix the namespace issue without introducing a generic breaking change that would be optimal I think.
Could we clarify the above questions?
receiver/k8sobjectsreceiver/testdata/e2e/collector/configmap.yaml
Outdated
Show resolved
Hide resolved
Thanks @ChrsMark for the feedback! You're right, changing the structure reported by the |
Cool! I think it's just fine fixing the namespace issue here and possibly file another issue to evaluate the "inconsistency" between the 2 modes. (in case we decide to change this, it should probably be done with a feature gate etc). To my mind there should be no difference in the 2 modes from users' perspective but maybe I miss the details here behind this decision. |
alright, then I'll adapt the current PR accordingly and will file an issue regarding the two different data structures - will let you know when this one is ready for another review :) |
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
pull
and watch
modek8s.namespace.name
attribute is set for objects retrieved using the watch
mode
Signed-off-by: Florian Bacher <[email protected]>
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.
lgtm
Signed-off-by: Florian Bacher <[email protected]>
Description
This PR ensures that the structure of log records generated by the k8sobjects receiver is the same, regardless of the mode (
watch
orpull
) being used. This also solves the issue of thek8s.namespace.name
attribute not being set for objects retrieved withwatch
mode.Link to tracking issue
Fixes #36352
Testing
Added unit tests and adapted e2e tests