Adding opencensus metrics #1061

jmcshane · 2021-04-17T04:27:58Z

Changes

Adding metrics for eventlisteners

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices
Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Release Notes

Eventlistener OpenCensus metrics

tekton-robot · 2021-04-17T04:30:16Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	79.5%	79.5%	-0.0
pkg/sink/metrics.go	Do not exist	0.0%

tekton-robot · 2021-04-20T14:09:35Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	79.5%	79.0%	-0.5
pkg/sink/metrics.go	Do not exist	38.1%
pkg/sink/sink.go	79.0%	79.2%	0.1

tekton-robot · 2021-04-20T14:10:39Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	79.5%	79.0%	-0.5
pkg/sink/metrics.go	Do not exist	38.1%
pkg/sink/sink.go	79.0%	79.2%	0.1

tekton-robot · 2021-04-20T14:19:06Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	79.5%	79.0%	-0.5
pkg/sink/metrics.go	Do not exist	78.6%
pkg/sink/sink.go	79.0%	79.2%	0.1

tekton-robot · 2021-04-20T14:31:06Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	79.5%	79.0%	-0.5
pkg/sink/metrics.go	Do not exist	78.6%
pkg/sink/sink.go	79.0%	79.2%	0.1

tekton-robot · 2021-04-20T21:07:02Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	79.5%	79.0%	-0.5
pkg/sink/metrics.go	Do not exist	78.6%
pkg/sink/sink.go	79.0%	79.2%	0.1

jmcshane · 2021-04-22T02:58:37Z

TODO:

Remove metrics port from service due to >1 replicas
Ensure metrics container port is set on deployment

docs/eventlistener-metrics.md

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go

dibyom · 2021-04-22T21:29:50Z

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go

+		ObjectMeta: metav1.ObjectMeta{Name: metrics.ConfigMapName()},
+		Data: map[string]string{
+			//TODO: Better nonempty config
+			"_example": "See tekton-pipelines namespace for valid values",


Yeah, we should probably have a more sensible default here.

So, one thing here is that this is a sensible default in that it will automatically select a prometheus exporter on 9090. It just needs to have one key so that the configmap watcher things the configmap is valid. See what we do for this in the install: https://github.com/tektoncd/triggers/blob/main/config/config-observability.yaml#L24

I could just copy the example value whole hog, but I didn't want to duplicate code too much.

ok, I'd change the message to say See config-observability in tekton-pipelines (or a link to the docs) for valid values

tekton-robot · 2021-04-25T00:16:36Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	79.5%	79.0%	-0.5
pkg/sink/metrics.go	Do not exist	78.6%
pkg/sink/sink.go	79.0%	79.2%	0.1

tekton-robot · 2021-04-25T00:17:15Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	79.5%	79.0%	-0.5
pkg/sink/metrics.go	Do not exist	78.6%
pkg/sink/sink.go	79.0%	79.2%	0.1

tekton-robot · 2021-04-25T00:23:04Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	79.5%	79.0%	-0.5
pkg/sink/metrics.go	Do not exist	78.6%
pkg/sink/sink.go	79.0%	79.2%	0.1

dibyom · 2021-04-28T14:24:16Z

pkg/sink/metrics.go

+}
+
+func (s *Sink) recordResourceCreation(resources []json.RawMessage) {
+	for _, rt := range resources {


how hard would it be to add some unit tests for the functions in this file? esp. this one to verify that we are adding the right metric?

definitely possible, ill take a look

tekton-robot · 2021-05-04T18:49:25Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	79.5%	79.0%	-0.5
pkg/sink/metrics.go	Do not exist	78.6%
pkg/sink/sink.go	79.0%	79.2%	0.1

tekton-robot · 2021-05-04T19:33:39Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	79.5%	79.0%	-0.5
pkg/sink/metrics.go	Do not exist	78.6%
pkg/sink/sink.go	79.0%	79.2%	0.1

dibyom · 2021-05-05T21:44:20Z

It looks like this PR now includes a couple of other commits? Could you rebase/squash from main? Otherwise, looks good

This PR creates an initial structure where the eventlistener sink is able to record metrics about its operation to a recorder that operates based off the same config structure that the triggers controller uses. The metrics recorded in this PR are the histogram of HTTP request duration and the count of resources created by this eventlistener, categorized by resource kind.

tekton-robot · 2021-05-06T02:12:04Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	79.5%	79.0%	-0.5
pkg/sink/metrics.go	Do not exist	78.6%
pkg/sink/sink.go	79.0%	79.2%	0.1

dibyom · 2021-05-06T15:10:09Z

/approve
/lgtm

tekton-robot · 2021-05-06T15:10:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dibyom]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Apr 17, 2021

tekton-robot requested review from bobcatfish and wlynch April 17, 2021 04:27

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 17, 2021

jmcshane force-pushed the feature/eventlistener-metrics branch 2 times, most recently from 7d4fc69 to b279a23 Compare April 20, 2021 14:09

jmcshane marked this pull request as ready for review April 20, 2021 14:09

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 20, 2021

jmcshane force-pushed the feature/eventlistener-metrics branch from b279a23 to af98ff7 Compare April 20, 2021 14:18

jmcshane force-pushed the feature/eventlistener-metrics branch from af98ff7 to b4ac1e2 Compare April 20, 2021 14:29

jmcshane mentioned this pull request Apr 20, 2021

Begin to address metrics in namespaced eventlistener #974

Closed

4 tasks

jmcshane force-pushed the feature/eventlistener-metrics branch from b4ac1e2 to e8a710f Compare April 20, 2021 21:05

dibyom reviewed Apr 22, 2021

View reviewed changes

jmcshane force-pushed the feature/eventlistener-metrics branch 2 times, most recently from 946b475 to 24a60db Compare April 25, 2021 00:15

jmcshane force-pushed the feature/eventlistener-metrics branch from 24a60db to e93bc49 Compare April 25, 2021 00:21

dibyom reviewed Apr 28, 2021

View reviewed changes

jmcshane force-pushed the feature/eventlistener-metrics branch from e93bc49 to 354c3db Compare May 4, 2021 18:40

tekton-robot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 4, 2021

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 4, 2021

jmcshane force-pushed the feature/eventlistener-metrics branch from 354c3db to b1cef01 Compare May 4, 2021 19:31

wlynch linked an issue May 5, 2021 that may be closed by this pull request

FR: Metrics for EventListener #736

Closed

jmcshane force-pushed the feature/eventlistener-metrics branch from b1cef01 to 860b287 Compare May 6, 2021 02:11

tekton-robot assigned dibyom May 6, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 6, 2021

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 6, 2021

tekton-robot merged commit 50293ad into tektoncd:main May 6, 2021

savitaashture mentioned this pull request May 11, 2021

Fix metrics port expose issue for triggers eventlistener #1086

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding opencensus metrics #1061

Adding opencensus metrics #1061

jmcshane commented Apr 17, 2021 •

edited

Loading

tekton-robot commented Apr 17, 2021

tekton-robot commented Apr 20, 2021

tekton-robot commented Apr 20, 2021

tekton-robot commented Apr 20, 2021

tekton-robot commented Apr 20, 2021

tekton-robot commented Apr 20, 2021

jmcshane commented Apr 22, 2021 •

edited

Loading

dibyom Apr 22, 2021

jmcshane Apr 25, 2021

dibyom Apr 29, 2021

tekton-robot commented Apr 25, 2021

tekton-robot commented Apr 25, 2021

tekton-robot commented Apr 25, 2021

dibyom Apr 28, 2021

jmcshane Apr 28, 2021

tekton-robot commented May 4, 2021

tekton-robot commented May 4, 2021

dibyom commented May 5, 2021

tekton-robot commented May 6, 2021

dibyom commented May 6, 2021

tekton-robot commented May 6, 2021

Adding opencensus metrics #1061

Adding opencensus metrics #1061

Conversation

jmcshane commented Apr 17, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Apr 17, 2021

tekton-robot commented Apr 20, 2021

tekton-robot commented Apr 20, 2021

tekton-robot commented Apr 20, 2021

tekton-robot commented Apr 20, 2021

tekton-robot commented Apr 20, 2021

jmcshane commented Apr 22, 2021 • edited Loading

dibyom Apr 22, 2021

Choose a reason for hiding this comment

jmcshane Apr 25, 2021

Choose a reason for hiding this comment

dibyom Apr 29, 2021

Choose a reason for hiding this comment

tekton-robot commented Apr 25, 2021

tekton-robot commented Apr 25, 2021

tekton-robot commented Apr 25, 2021

dibyom Apr 28, 2021

Choose a reason for hiding this comment

jmcshane Apr 28, 2021

Choose a reason for hiding this comment

tekton-robot commented May 4, 2021

tekton-robot commented May 4, 2021

dibyom commented May 5, 2021

tekton-robot commented May 6, 2021

dibyom commented May 6, 2021

tekton-robot commented May 6, 2021

jmcshane commented Apr 17, 2021 •

edited

Loading

jmcshane commented Apr 22, 2021 •

edited

Loading