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

Add events metricset for kubernetes metricbeat module #4315

Merged
merged 3 commits into from May 18, 2017

Conversation

Projects
None yet
6 participants
@vjsamuel
Copy link
Contributor

commented May 15, 2017

This PR adds a kubernetes events metricset to the kubernetes metricbeat module. Events are polled from the kubernetes API server and are written into the outputs interface.

@vjsamuel vjsamuel changed the title Adding events metricset for kubernetes metricbeat module WIP: Adding events metricset for kubernetes metricbeat module May 15, 2017

@vjsamuel

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

@ruflin, vendoring needs to be decided as im getting these info messages:

2017/05/15 01:06:58 proto: duplicate proto type registered: github.com/ericchiang.k8s.apis.extensions.v1beta1.DaemonSetList
2017/05/15 01:06:58 proto: duplicate proto type registered: github.com/ericchiang.k8s.apis.extensions.v1beta1.DaemonSetSpec
2017/05/15 01:06:58 proto: duplicate proto type registered: github.com/ericchiang.k8s.apis.extensions.v1beta1.DaemonSetStatus
2017/05/15 01:06:58 proto: duplicate proto type registered: github.com/ericchiang.k8s.apis.extensions.v1beta1.DaemonSetUpdateStrategy
2017/05/15 01:06:58 proto: duplicate proto type registered: github.com/ericchiang.k8s.apis.extensions.v1beta1.Deployment

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2017

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

1 similar comment
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2017

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

@andrewkroh
Copy link
Member

left a comment

LGTM, did a quick scan of the metricset implementation only.

// Part of new is also setting up the configuration by processing additional
// configuration entries if needed.
func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
logp.Warn("EXPERIMENTAL: The kubernetes events metricset is experimental")

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh May 15, 2017

Member

Use logp.Experimental.

This comment has been minimized.

Copy link
@vjsamuel

vjsamuel May 15, 2017

Author Contributor

Done. We would need to update the metricset generator as this was generated code.

return nil, fmt.Errorf("fail to unpack the kubernetes events configuration: %s", err)
}

err = validate(config)

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh May 15, 2017

Member

go-ucfg allows a config type to implement a Validate() error function that is invoked automatically on when unpacking.

This comment has been minimized.

Copy link
@vjsamuel

vjsamuel May 15, 2017

Author Contributor

Done.

m.watcher.Stop()
return
case msg := <-m.watcher.eventQueue:
// Ignore events that are deleted

This comment has been minimized.

Copy link
@exekias

exekias May 15, 2017

Member

Why are you ignoring deletion events? I guess someone could be interested on them, perhaps we should offer some fine grained filter for desired events?

This comment has been minimized.

Copy link
@vjsamuel

vjsamuel May 15, 2017

Author Contributor

delete, in case of events is to just delete the API Event object. it carries no significance from monitoring perspective.

@vjsamuel vjsamuel changed the title WIP: Adding events metricset for kubernetes metricbeat module Add events metricset for kubernetes metricbeat module May 15, 2017

@vjsamuel vjsamuel force-pushed the vjsamuel:kubernetes_events branch 2 times, most recently from 2e53faf to c87167f May 15, 2017

@ruflin
Copy link
Collaborator

left a comment

Thanks for the PR. I left a few comments mainly related to naming conventions.

One thing that seems to be missing is adding it to the config file.

From which of the two services which we connect to in the kubernetes module at the moment will this info be fetched? What is the installation pattern?

@@ -6040,6 +6040,194 @@ Used inodes
[float]
== events Fields

This comment has been minimized.

Copy link
@ruflin

ruflin May 16, 2017

Collaborator

The metricset should probably be called event and not events to be in line with our naming convention.



[float]
=== kubernetes.events.firstOccurrenceTimestamp

This comment has been minimized.

Copy link
@ruflin

ruflin May 16, 2017

Collaborator

I would suggest to change this to *.event.timestamp.first_occurenceand below to *.event.timestamp.last_occurence.



[float]
=== kubernetes.events.metadata.creationTimestamp

This comment has been minimized.

Copy link
@ruflin

ruflin May 16, 2017

Collaborator

timestamp.created, timestamp.deleted



[float]
=== kubernetes.events.metadata.resourceVersion

This comment has been minimized.

Copy link
@ruflin

ruflin May 16, 2017

Collaborator

resource_version or version.resource



[float]
=== kubernetes.events.metadata.selfLink

This comment has been minimized.

Copy link
@ruflin

ruflin May 16, 2017

Collaborator

self_link



[float]
=== kubernetes.events.tags.host

This comment has been minimized.

Copy link
@ruflin

ruflin May 16, 2017

Collaborator

Why is this under tags



[float]
=== kubernetes.events.tags.pod

This comment has been minimized.

Copy link
@ruflin

ruflin May 16, 2017

Collaborator

What is this under tags. Don't we have a common namespace for these?

This comment has been minimized.

Copy link
@vjsamuel

vjsamuel May 16, 2017

Author Contributor

these are dimensions that can be used to better query information. base implementation off of which I went with is here:
https://github.com/kubernetes/heapster/blob/master/events/sinks/elasticsearch/driver.go#L76

)

// EventWatcher is a controller that synchronizes Pods.
type EventWatcher struct {

This comment has been minimized.

Copy link
@ruflin

ruflin May 16, 2017

Collaborator

You could call this just Watcher as it is already in the event package.

if err != nil {
//if listing fails try again after sometime
logp.Err("kubernetes: List API error %v", err)
time.Sleep(time.Second)

This comment has been minimized.

Copy link
@ruflin

ruflin May 16, 2017

Collaborator

Based on what did you select this timeout? Perhaps a comment could be helpful here.

//watch events failures should be logged and gracefully failed over as metadata retrieval
//should never stop.
logp.Err("kubernetes: Watching API eror %v", err)
time.Sleep(time.Second)

This comment has been minimized.

Copy link
@ruflin

ruflin May 16, 2017

Collaborator

see above

@vjsamuel vjsamuel force-pushed the vjsamuel:kubernetes_events branch from c87167f to 463299c May 16, 2017

- module: kubernetes
metricsets:
- event
kube_config: ${HOME}/.kube/config

This comment has been minimized.

Copy link
@ruflin

ruflin May 17, 2017

Collaborator
  • Is ${HOME} always set in these environments?
  • You left out the hosts: [...] part. Is that on purpose?

This comment has been minimized.

Copy link
@vjsamuel

vjsamuel May 17, 2017

Author Contributor

as per @exekias we can default in_cluster to true. this metricset doesnt rely on hosts by default. we can add documentation on various ways to configure it similar to the kubernetes processor.

},
"message": "Created pod: prometheus-2552087900-9fxh6",
"metadata": {
"annotations": null,

This comment has been minimized.

Copy link
@ruflin

ruflin May 17, 2017

Collaborator

Would be nice to not send fields if they are null

This comment has been minimized.

Copy link
@vjsamuel

vjsamuel May 17, 2017

Author Contributor

done

# Kubernetes events
- module: kubernetes
metricsets:
- event

This comment has been minimized.

Copy link
@ruflin

ruflin May 17, 2017

Collaborator

I remember @exekias added somewhere that all state_* prefixed metricset must be installed once in contrast to the other ones. If this is installed global (my assumption) should this also have a state_ prefix?

NOTE: In general I think we need to find a better solution for this in the long run but in the short term we should try to be consistent.

This comment has been minimized.

Copy link
@exekias

exekias May 17, 2017

Member

I think event (or events) is ok in this case, this is something unrelated to kube-state-metrics, state_ metricsets get their name from it

metricsets:
- event
kube_config: ${HOME}/.kube/config
in_cluster: false

This comment has been minimized.

Copy link
@exekias

exekias May 17, 2017

Member

perhaps we should leave in_cluster: true as default? I guess it will be the most common case

This comment has been minimized.

Copy link
@vjsamuel

vjsamuel May 17, 2017

Author Contributor

will default to true

# Kubernetes events
- module: kubernetes
metricsets:
- event

This comment has been minimized.

Copy link
@exekias

exekias May 17, 2017

Member

I think event (or events) is ok in this case, this is something unrelated to kube-state-metrics, state_ metricsets get their name from it

"deleted": eve.Metadata.DeletionTimestamp,
},
"name": eve.Metadata.Name,
"namespace": eve.Metadata.Namespace,

This comment has been minimized.

Copy link
@exekias

exekias May 17, 2017

Member

this one can ve moved to kubernetes.namespace, using mb.ModuleKey. WDYT?

This comment has been minimized.

Copy link
@vjsamuel

vjsamuel May 17, 2017

Author Contributor

we would need to do this across all metricsets to establish consistency. lets do it in a follow-up PR all in one go.

@vjsamuel vjsamuel force-pushed the vjsamuel:kubernetes_events branch from 463299c to a5e3f48 May 17, 2017

@vjsamuel vjsamuel force-pushed the vjsamuel:kubernetes_events branch from a5e3f48 to 3267969 May 18, 2017

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented May 18, 2017

jenkins, test it

SyncPeriod time.Duration `config:"sync_period"`
}

type Enabled struct {

This comment has been minimized.

Copy link
@elasticmachine

elasticmachine May 18, 2017

Collaborator

[golint] reported by reviewdog 🐶
exported type Enabled should have comment or be unexported

Enabled bool `config:"enabled"`
}

type PluginConfig []map[string]common.Config

This comment has been minimized.

Copy link
@elasticmachine

elasticmachine May 18, 2017

Collaborator

[golint] reported by reviewdog 🐶
exported type PluginConfig should have comment or be unexported


import "time"

type ObjectMeta struct {

This comment has been minimized.

Copy link
@elasticmachine

elasticmachine May 18, 2017

Collaborator

[golint] reported by reviewdog 🐶
exported type ObjectMeta should have comment or be unexported

UID string `json:"uid"`
}

type Event struct {

This comment has been minimized.

Copy link
@elasticmachine

elasticmachine May 18, 2017

Collaborator

[golint] reported by reviewdog 🐶
exported type Event should have comment or be unexported


}

func (w *Watcher) Run() {

This comment has been minimized.

Copy link
@elasticmachine

elasticmachine May 18, 2017

Collaborator

[golint] reported by reviewdog 🐶
exported method Watcher.Run should have comment or be unexported


}

func (w *Watcher) Stop() {

This comment has been minimized.

Copy link
@elasticmachine

elasticmachine May 18, 2017

Collaborator

[golint] reported by reviewdog 🐶
exported method Watcher.Stop should have comment or be unexported

@exekias exekias merged commit cc0f165 into elastic:master May 18, 2017

6 checks passed

CLA Commit author has signed the CLA
Details
codecov/patch 0.56% of diff hit (within 100% threshold of 63.39%)
Details
codecov/project 63.1% (-0.29%) compared to 97de042
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details

@exekias exekias referenced this pull request May 18, 2017

Open

Follow up k8s events metricset #4347

0 of 3 tasks complete

@monicasarbu monicasarbu added review and removed in progress labels May 23, 2017

ramon-garcia added a commit to ramon-garcia/beats that referenced this pull request Dec 5, 2017

Add events metricset for kubernetes metricbeat module (elastic#4315)
* Adding events metricset for kubernetes metricbeat module

* Moving k8s client to top level vendor

* Incorporating review comments

athom added a commit to athom/beats that referenced this pull request Jan 25, 2018

Add events metricset for kubernetes metricbeat module (elastic#4315)
* Adding events metricset for kubernetes metricbeat module

* Moving k8s client to top level vendor

* Incorporating review comments

@vjsamuel vjsamuel deleted the vjsamuel:kubernetes_events branch Mar 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.