Skip to content
This repository was archived by the owner on Jun 24, 2020. It is now read-only.

Add metrics about KnativeServing CR edits #233

Merged
knative-prow-robot merged 2 commits into from
Dec 7, 2019
Merged

Add metrics about KnativeServing CR edits #233

knative-prow-robot merged 2 commits into from
Dec 7, 2019

Conversation

@jihuin
Copy link
Contributor

@jihuin jihuin commented Nov 22, 2019 •
edited
Loading

Proposed Changes

Serving operator can emit metrics on KnativeServing CR edits, including creation, modification and deletion.

Steps to test metrics with Prometheus exporter (default):

  1. Forward the local port 9090 to the port 9090 on operator pod by running
    kubectl port-forward ${{operator_pod_name}} 9090
  2. Check the metrics by visiting localhost:9090/metrics

Release Note

NONE

@googlebot googlebot added the cla: yes Author(s) signed a CLA. label Nov 22, 2019
@jihuin
Copy link
Contributor Author

jihuin commented Nov 22, 2019

/assign @trshafer @anniefu @garron

trshafer
trshafer reviewed Nov 22, 2019
View reviewed changes
anniefu
anniefu reviewed Nov 22, 2019
View reviewed changes
anniefu
anniefu reviewed Nov 22, 2019
View reviewed changes
@jihuin jihuin changed the title Add metrics about KnativeServing CR install and deployment spin-up Add metrics about KnativeServing CR edits Nov 27, 2019
trshafer
trshafer reviewed Dec 3, 2019
View reviewed changes
pkg/reconciler/knativeserving/knativeserving_controller.go Outdated
r.config.DeleteAll(&metav1.DeleteOptions{})
// Report the KnativeServing edit when deleting an existed KnativeServing
if _, ok := r.servings[key]; ok {
r.StatsReporter.ReportKnativeServingEdit(key)
Copy link
Contributor

@trshafer trshafer Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test?

pkg/reconciler/knativeserving/knativeserving_controller.go Outdated
newGen := original.Generation
oldGen, ok := r.servings[key]
if (ok && newGen == oldGen+1) || (!ok && newGen == 1) {
r.StatsReporter.ReportKnativeServingEdit(key)
Copy link
Contributor

@trshafer trshafer Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the generation be attached?

Copy link
Contributor Author

@jihuin jihuin Dec 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Attaching the generation may be not that useful for the metrics only counting the number of changes.

anniefu
anniefu reviewed Dec 3, 2019
View reviewed changes
anniefu
anniefu reviewed Dec 3, 2019
View reviewed changes
anniefu
anniefu reviewed Dec 3, 2019
View reviewed changes
anniefu
anniefu reviewed Dec 3, 2019
View reviewed changes
anniefu
anniefu reviewed Dec 3, 2019
View reviewed changes
anniefu
anniefu reviewed Dec 3, 2019
View reviewed changes
anniefu
anniefu reviewed Dec 3, 2019
View reviewed changes
anniefu
anniefu reviewed Dec 3, 2019
View reviewed changes
anniefu
anniefu reviewed Dec 3, 2019
View reviewed changes
anniefu
anniefu reviewed Dec 3, 2019
View reviewed changes
@jihuin
Create stats reporter and test
bf83702 
Add stats reporter to reconciler base

Report metrics about KnativeServing CR change
anniefu
anniefu reviewed Dec 4, 2019
View reviewed changes
anniefu
anniefu reviewed Dec 4, 2019
View reviewed changes
@knative-metrics-robot
Copy link

knative-metrics-robot commented Dec 5, 2019

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

File Old Coverage New Coverage Delta
pkg/reconciler/reconciler.go 100.0% 94.4% -5.6
pkg/reconciler/stats_reporter.go Do not exist 75.0%

@anniefu
Copy link
Contributor

anniefu commented Dec 5, 2019

Could you test with stackdriver by setting these in config-observability?

    metrics.backend-destination: stackdriver
    metrics.request-metrics-backend-destination: stackdriver
    metrics.allow-stackdriver-custom-metrics: "true"

Otherwise,
/lgtm

@jihuin
Copy link
Contributor Author

jihuin commented Dec 5, 2019 •
edited
Loading

@anniefu Tested with prometheus and stackdriver, and both worked as expected.

@trshafer
Copy link
Contributor

trshafer commented Dec 7, 2019

/lgtm
/approve

@knative-prow-robot
Copy link
Contributor

knative-prow-robot commented Dec 7, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jihuin, trshafer

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

The pull request process is described here

Details 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

@knative-prow-robot knative-prow-robot merged commit 180e4e7 into knative:master Dec 7, 2019
@jihuin jihuin deleted the metrics branch December 27, 2019 18:55
@jcrossley3 jcrossley3 mentioned this pull request May 8, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@bbrowning bbrowning Awaiting requested review from bbrowning

3 more reviewers

@trshafer trshafer trshafer left review comments

@mattmoor mattmoor mattmoor left review comments

@anniefu anniefu anniefu left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@trshafer trshafer

@garron garron

@anniefu anniefu

Labels

approved cla: yes Author(s) signed a CLA. lgtm size/L

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@jihuin @knative-metrics-robot @anniefu @trshafer @knative-prow-robot @mattmoor @garron @googlebot