Skip to content
This repository has been archived by the owner on Jun 19, 2022. It is now read-only.

Limit the values of event_type in Broker metrics #1352

Conversation

zargarpur
Copy link
Contributor

@zargarpur zargarpur commented Jun 25, 2020

With custom events, these values can be any string which impacts the overall cardinality of the metrics.

We're instead going to limit these values to be any GCP event type, as defined by the "google.cloud" prefix. And include some values used in some E2E tests. All other event types will default to being labeled as "custom"

Trigger metrics will continue to label filter type as "any" if there is no filter type defined.

Broker metrics won't include specific custom event types in Stackdriver other than GCP defined ones, and will instead default to "custom"

With custom events, these values can be any string which impacts the
overall cardinality of the metrics.

We're instead going to limit these values to be any GCP event type, as
defined by the "com.google.cloud" prefix. And include some values used
in some E2E tests. All other event types will default to being labeled
as "custom"

Trigger metrics will continue to label filter type as "any" if there is
no filter type defined.
@googlebot googlebot added the cla: yes (override cla status due to multiple authors bug) label Jun 25, 2020
@zargarpur zargarpur requested a review from Harwayne June 25, 2020 23:40
pkg/metrics/tags_test.go Outdated Show resolved Hide resolved
@ian-mi
Copy link
Contributor

ian-mi commented Jun 25, 2020

Is there anything preventing a user from creating custom event types with a com.google.cloud prefix?

pkg/metrics/tags.go Outdated Show resolved Hide resolved
@nachocano
Copy link
Member

nachocano commented Jun 26, 2020 via email

@nachocano
Copy link
Member

/approve
/lgtm
/hold
in case others have any other comment. Unhold otherwise

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nachocano, zargarpur

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:

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

@nachocano
Copy link
Member

/hold cancel

@zargarpur
Copy link
Contributor Author

/hold

@nachocano
Copy link
Member

/hold

@zargarpur zargarpur changed the title Limit the values of event_type & filter_type in Broker & Trigger metrics Limit the values of event_type in Broker metrics Jun 26, 2020
@ian-mi
Copy link
Contributor

ian-mi commented Jun 26, 2020

/lgtm
thanks!

@nachocano
Copy link
Member

/hold cancel

@knative-metrics-robot
Copy link

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

File Old Coverage New Coverage Delta
pkg/metrics/tags.go Do not exist 100.0%

@nachocano
Copy link
Member

nachocano commented Jun 27, 2020 via email

@nachocano
Copy link
Member

nachocano commented Jun 27, 2020 via email

@knative-test-reporter-robot

The following jobs failed:

Test name Triggers Retries
pull-google-knative-gcp-wi-tests pull-google-knative-gcp-wi-tests
pull-google-knative-gcp-wi-tests
pull-google-knative-gcp-wi-tests
pull-google-knative-gcp-wi-tests
pull-google-knative-gcp-wi-tests
3/3

Job pull-google-knative-gcp-wi-tests expended all 3 retries without success.

@nachocano
Copy link
Member

nachocano commented Jun 27, 2020 via email

@knative-prow-robot knative-prow-robot merged commit 189bcad into google:master Jun 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved cla: yes (override cla status due to multiple authors bug) lgtm size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants