[chore] [receiver/k8sobjects] Extract logic for watching/pulling K8s resources into separate package #43108
Conversation
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
# Conflicts: # receiver/k8sobjectsreceiver/config.go # receiver/k8sobjectsreceiver/receiver.go
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
bacherfl
changed the title
Signed-off-by: Florian Bacher <[email protected]>
bacherfl
requested review from
a team,
ChrsMark,
TylerHelmuth and
dmitryax
as code owners
ChrsMark
left a comment
ChrsMark
left a comment
There was a problem hiding this comment.
Thank's for working on this!
Overall looks good with some nits.
|
|
||
| defaultPullInterval time.Duration = time.Hour | ||
| defaultMode mode = PullMode | ||
| defaultResourceVersion = "1" |
There was a problem hiding this comment.
good catch, the defaultResourceVersion is not used anymore. will remove that
| }, | ||
| ) | ||
| } | ||
| return nil, fmt.Errorf("invalid observer mode mode: %s", object.Mode) |
There was a problem hiding this comment.
| return nil, fmt.Errorf("invalid observer mode mode: %s", object.Mode) | |
| return nil, fmt.Errorf("invalid observer mode: %s", object.Mode) |
| o.logger.Error("error in watching object", | ||
| zap.String("resource", o.config.Gvr.String()), | ||
| zap.Error(err)) | ||
| return true |
There was a problem hiding this comment.
Should that still be true given that the watch was not successful?
There was a problem hiding this comment.
i think you are right - here it would make sense to return false instead, to trigger a restart attempt in case of an error
| // Copyright The OpenTelemetry Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package pull // import "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/k8sobjectsreceiver/observer/pull" |
There was a problem hiding this comment.
Is the plan to further move this common code-base to a shared place? Otherwise it would be weird to import in the k8sevents receiver this code from the k8sobjects receiver, no?
Maybe we can use either pkg or internal? Probably internal since those are not expected to be used outside of this repository?
There was a problem hiding this comment.
good point - definitely makes sense to move that there. As this will be a new module within internal, is it ok if i set the code owners of that module to the ones that are currently listed in the k8sobjects receiver?
There was a problem hiding this comment.
it seems like we will have to split the PR in this case, since we need to have the new module in internal pushed to the main branch, in order to be able to reference it in the k8sobserver go.mod file
There was a problem hiding this comment.
Happy to hear what other think here before doing it. @TylerHelmuth @dmitryax any thoughts on this?
Thanks for the feedback @ChrsMark! I will address the comments and update the PR in the next few days |
Signed-off-by: Florian Bacher <[email protected]>
…er-code' into feat/40825/reusable-k8s-receiver-code
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]>
…and its sub-packages Signed-off-by: Florian Bacher <[email protected]>
…andling 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]>
Signed-off-by: Florian Bacher <[email protected]>
3 participants
Description
This PR refactors the
k8sobjectsreceiver by extracting the components responsible for receiving the K8s resources in the cluster into a separate package with anObserverinterface, and the pull/watch based implementations thereof.This package can then also be used by the
k8seventsreceiver, which would then only need to format the received events to conform with the semantic conventions.Link to tracking issue
Part of #40825 - next steps as part of this issue would be to also make use of the observer components in the k8seventsreceiver
Testing
Added tests for the new components. The tests for the receiver itself could be refactored to not use the concrete implementations of the Observer interface, but that should probably be done as a follow up to not make this PR too large