Add k8s observer #185

jrcamp · 2020-04-23T21:52:19Z

This adds a k8s observer as well as reworking some of the observer data
structures. Work is still ongoing to add an end to end test to validate it
against a real k8s instance and dynamically start a real receiver.

Before Endpoint was an interface with methods like ID(), Target(), etc.
but these had a smell because they were acting as simple data getters for
struct values. This changes Endpoint to be a struct itself that has members
like ID and Target. It has a member Details that can currently either be
a Pod (ie just has an IP address) or a Port (has a pod IP but also an
associated port). Currently this Details is an interface{} as there are
currently no methods in which to base an interface off of. This is basically
due to a limitation in Go which doesn't have true class inheritance. It is no
worse than before since when Endpoint was an interface the consumer would
have to type switch to check what concrete type it was (pod, port, etc.). I
have tried structuring it many ways and this is the least worse I've found thus
far.

This adds a k8s observer as well as reworking some of the observer data structures. Work is still ongoing to add an end to end test to validate it against a real k8s instance and dynamically start a real receiver. Before Endpoint was an interface with methods like `ID()`, `Target()`, etc. but these had a smell because they were acting as simple data getters for struct values. This changes Endpoint to be a struct itself that has members like `ID` and `Target`. It has a member `Details` that can currently either be a Pod (ie just has an IP address) or a Port (has a pod IP but also an associated port). Currently this `Details` is an `interface{}` as there are currently no methods in which to base an interface off of. This is basically due to a limitation in Go which doesn't have true class inheritance. It is no worse than before since when `Endpoint` was an interface the consumer would have to type switch to check what concrete type it was (pod, port, etc.). I have tried structuring it many ways and this is the least worse I've found thus far.

tigrannajaryan · 2020-04-24T18:42:19Z

extension/observer/k8s/config.go

+type Config struct {
+	configmodels.ExtensionSettings `mapstructure:",squash"`
+
+	// For example, node name can be passed to each agent with the downward API as follows


What is a "downward API"?

It's a kubernetes thing https://kubernetes.io/docs/tasks/inject-data-application/downward-api-volume-expose-pod-information/#the-downward-api

tigrannajaryan · 2020-04-24T18:42:44Z

extension/observer/k8s/config.go

+	// env:
+	//   - name: K8S_NODE_NAME
+	//     valueFrom:
+	//       fieldRef:
+	//         fieldPath: spec.nodeName
+	//
+	// Then set this value to ${K8S_NODE_NAME}


I do not understand this example. Please clarify. What does env signify here?

It's part of a pod spec. Should make more sense based on the link above but I can expand the comment.

tigrannajaryan · 2020-04-24T18:50:49Z

extension/observer/k8s/factory.go

+	return &Factory{createK8sConfig: func() (*rest.Config, error) {
+		restConfig, err := rest.InClusterConfig()
+		if err != nil {
+			return nil, fmt.Errorf("failed creating Kubernetes in-cluster REST config: %v", err)


It appears that this will result in "failed creating Kubernetes in-cluster REST config" repeated twice when called from CreateExtension()

extension/observer/k8s/handler.go

tigrannajaryan · 2020-04-24T19:02:44Z

extension/observer/k8s/handler.go

+
+// OnAdd is called in response to a pod being added.
+func (h *handler) OnAdd(obj interface{}) {
+	pod := obj.(*v1.Pod)


Is it guaranteed for obj to be v1.Pod? Perhaps be defensive and check?

The kubernetes cache syncers are of a single type and we only sync pods so not too worried about any other type sneaking in.

I would still be careful. I assume checking the interface has no or little cost. I get an eye pain from this unprotected conversion :-)
If for some reason you do not want to add the check I would document that obj is expected to be of particular interface and why the expectation.

tigrannajaryan · 2020-04-24T19:03:59Z

extension/observer/k8s/handler.go

+	oldEndpoints := map[string]observer.Endpoint{}
+	newEndpoints := map[string]observer.Endpoint{}
+
+	for _, e := range h.convertPodToEndpoints(oldObj.(*v1.Pod)) {


Same here. Maybe be defensive and check the type assertion?

tigrannajaryan · 2020-04-24T19:05:04Z

extension/observer/k8s/handler.go

+
+	var removedEndpoints, updatedEndpoints, addedEndpoints []observer.Endpoint
+
+	for _, e := range newEndpoints {


Would be good to add a comment to explain what this loop is doing.

tigrannajaryan · 2020-04-24T19:06:36Z

extension/observer/observer.go

-	Labels() map[string]string
+	Target string
+	// Details contains additional context about the endpoint such as a Pod or Port.
+	Details interface{}


I know you mentioned this in the PR but just to double check is there any trait that Details must exhibit that we can define an interface on? Perhaps it has to have a Name?

I think in the future it might have something like ToMap() map[string]string that returns a map of the object using semantic conventions but it's not needed yet. If something like Name would be common across all Details then it would go into the Endpoint instead I think. I considered Type() but that's really just duplicating the Go type so not much help.

Co-Authored-By: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>

…r-contrib into k8s

bogdandrutu · 2020-04-27T14:46:20Z

extension/observer/k8s/factory.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package k8s


I would use k8s_observer as package

tigrannajaryan · 2020-04-27T22:36:36Z

Please resolve the conflict.

tigrannajaryan

LGTM, except a couple unchecked interface type assertions, which make me a bit uncomfortable and appear to be against our "don't crash" recommendation: https://github.com/open-telemetry/opentelemetry-collector/blob/master/CONTRIBUTING.md#do-not-crash-after-startup

tigrannajaryan · 2020-04-27T22:38:34Z

extension/observer/k8s/handler.go

+
+// OnAdd is called in response to a pod being added.
+func (h *handler) OnAdd(obj interface{}) {
+	pod := obj.(*v1.Pod)


I would still be careful. I assume checking the interface has no or little cost. I get an eye pain from this unprotected conversion :-)
If for some reason you do not want to add the check I would document that obj is expected to be of particular interface and why the expectation.

jrcamp · 2020-04-28T15:03:19Z

LGTM, except a couple unchecked interface type assertions, which make me a bit uncomfortable and appear to be against our "don't crash" recommendation: https://github.com/open-telemetry/opentelemetry-collector/blob/master/CONTRIBUTING.md#do-not-crash-after-startup

I will change it so you can sleep at night. :-P

tigrannajaryan · 2020-04-28T16:45:30Z

LGTM, except a couple unchecked interface type assertions, which make me a bit uncomfortable and appear to be against our "don't crash" recommendation: https://github.com/open-telemetry/opentelemetry-collector/blob/master/CONTRIBUTING.md#do-not-crash-after-startup

I will change it so you can sleep at night. :-P

Thank you! :-)

tigrannajaryan

Please fix the build (something about goimports).

jrcamp · 2020-04-28T18:26:25Z

@tigrannajaryan build is green now

tigrannajaryan · 2020-04-28T22:09:51Z

@bogdandrutu are you OK or you need to review more?

bogdandrutu · 2020-04-29T14:59:29Z

Ship it :)

* Add k8s observer This adds a k8s observer as well as reworking some of the observer data structures. Work is still ongoing to add an end to end test to validate it against a real k8s instance and dynamically start a real receiver. Before Endpoint was an interface with methods like `ID()`, `Target()`, etc. but these had a smell because they were acting as simple data getters for struct values. This changes Endpoint to be a struct itself that has members like `ID` and `Target`. It has a member `Details` that can currently either be a Pod (ie just has an IP address) or a Port (has a pod IP but also an associated port). Currently this `Details` is an `interface{}` as there are currently no methods in which to base an interface off of. This is basically due to a limitation in Go which doesn't have true class inheritance. It is no worse than before since when `Endpoint` was an interface the consumer would have to type switch to check what concrete type it was (pod, port, etc.). I have tried structuring it many ways and this is the least worse I've found thus far. * fix go.mod * remove e2e test that is not ready * Apply suggestions from code review Co-Authored-By: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com> * review updates * review updates * rename package * cleanup deps * fix type changes * fix linting * fix build Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>

* Add license automatically * Add addlicense to make default goal * Add dependency to internal tools

jrcamp requested review from bogdandrutu, flands, owais, pjanotti, rghetia, songy23 and tigrannajaryan as code owners April 23, 2020 21:52

jrcamp added 3 commits April 23, 2020 17:56

fix go.mod

102acf8

Merge remote-tracking branch 'upstream/master' into k8s

988cb2d

remove e2e test that is not ready

15de07d

tigrannajaryan reviewed Apr 24, 2020

View reviewed changes

jrcamp and others added 3 commits April 24, 2020 16:05

Apply suggestions from code review

ae33993

Co-Authored-By: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>

review updates

417dfce

Merge branch 'k8s' of github.com-jrcamp:jrcamp/opentelemetry-collecto…

73a653c

…r-contrib into k8s

bogdandrutu reviewed Apr 27, 2020

View reviewed changes

tigrannajaryan reviewed Apr 27, 2020

View reviewed changes

jrcamp added 5 commits April 28, 2020 12:26

review updates

5fd3ff2

rename package

794abc5

Merge remote-tracking branch 'upstream/master' into k8s

2f5f110

cleanup deps

5d634a1

fix type changes

7089a45

jrcamp requested review from bogdandrutu and tigrannajaryan April 28, 2020 16:42

tigrannajaryan approved these changes Apr 28, 2020

View reviewed changes

jrcamp added 2 commits April 28, 2020 12:56

fix linting

9291c34

fix build

d98e698

bogdandrutu merged commit ba49ec3 into open-telemetry:master Apr 29, 2020

jrcamp mentioned this pull request May 18, 2020

REQUEST: New membership for jrcamp open-telemetry/community#349

Closed

6 tasks

mxiamxia referenced this pull request in mxiamxia/opentelemetry-collector-contrib Jul 22, 2020

Check license existence, add license otherwise (#185)

9233b6c

* Add license automatically * Add addlicense to make default goal * Add dependency to internal tools

straussb pushed a commit to straussb/opentelemetry-collector-contrib that referenced this pull request Mar 26, 2024

Add InstanceType to GPU metrics (open-telemetry#185)

34bd73a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add k8s observer #185

Add k8s observer #185

jrcamp commented Apr 23, 2020

tigrannajaryan Apr 24, 2020

jrcamp Apr 24, 2020

tigrannajaryan Apr 24, 2020

jrcamp Apr 24, 2020

tigrannajaryan Apr 24, 2020

jrcamp Apr 24, 2020

tigrannajaryan Apr 24, 2020

jrcamp Apr 24, 2020

tigrannajaryan Apr 27, 2020

tigrannajaryan Apr 24, 2020

tigrannajaryan Apr 24, 2020

jrcamp Apr 24, 2020

tigrannajaryan Apr 24, 2020

jrcamp Apr 24, 2020

bogdandrutu Apr 27, 2020

tigrannajaryan commented Apr 27, 2020

tigrannajaryan left a comment

tigrannajaryan Apr 27, 2020

jrcamp commented Apr 28, 2020

tigrannajaryan commented Apr 28, 2020

tigrannajaryan left a comment

jrcamp commented Apr 28, 2020

tigrannajaryan commented Apr 28, 2020

bogdandrutu commented Apr 29, 2020


		var removedEndpoints, updatedEndpoints, addedEndpoints []observer.Endpoint

		for _, e := range newEndpoints {

Add k8s observer #185

Add k8s observer #185

Conversation

jrcamp commented Apr 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigrannajaryan commented Apr 27, 2020

tigrannajaryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrcamp commented Apr 28, 2020

tigrannajaryan commented Apr 28, 2020

tigrannajaryan left a comment

Choose a reason for hiding this comment

jrcamp commented Apr 28, 2020

tigrannajaryan commented Apr 28, 2020

bogdandrutu commented Apr 29, 2020