Skip to content

Add metrics to webhook package#503

Merged
knative-prow-robot merged 12 commits into
knative:masterfrom
anniefu:webhookMetrics
Jul 8, 2019
Merged

Add metrics to webhook package#503
knative-prow-robot merged 12 commits into
knative:masterfrom
anniefu:webhookMetrics

Conversation

@anniefu
Copy link
Copy Markdown
Contributor

@anniefu anniefu commented Jul 2, 2019 •
edited
Loading

Metrics added:

  • "request_count": the number of AdmissionReview requests the webhook received and responded to (invalid requests are ignored)
  • "request_latencies": the time in ms it took from receiving the request to sending the response

Other:

  • Refactor reused metrics testing code into "metricstest" package that can be shared

See knative/serving#1279

@anniefu
Add metrics to webhook package
c5939bb 
Add metricstest package for shared helper functions for testing metrics
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 2, 2019
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 2, 2019
vagababov
vagababov reviewed Jul 2, 2019
View reviewed changes
Comment thread webhook/stats_reporter.go Outdated
Comment thread webhook/stats_reporter.go Outdated
vagababov
vagababov reviewed Jul 2, 2019
View reviewed changes
Comment thread metrics/metricstest/metricstest.go Outdated
Comment thread metrics/metricstest/metricstest.go Outdated
Comment thread metrics/metricstest/metricstest.go
Comment thread metrics/metricstest/metricstest.go Outdated
Comment thread metrics/metricstest/metricstest.go Outdated
Comment thread metrics/metricstest/metricstest.go Outdated
Comment thread metrics/metricstest/metricstest.go Outdated
@yt3liu
Copy link
Copy Markdown
Contributor

yt3liu commented Jul 3, 2019

/retest

vagababov
vagababov reviewed Jul 3, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@vagababov vagababov left a comment

Choose a reason for hiding this comment

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

In general looks fine.

Comment thread metrics/metricstest/metricstest.go Outdated
Comment thread metrics/metricstest/metricstest.go Outdated
@anniefu
Copy link
Copy Markdown
Contributor Author

anniefu commented Jul 3, 2019

@vagababov thanks for taking the time to review!

vagababov
vagababov reviewed Jul 3, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@vagababov vagababov left a comment

Choose a reason for hiding this comment

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

/lgtm

Comment thread metrics/metricstest/metricstest.go Outdated
Comment thread webhook/stats_reporter_test.go Outdated
@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 3, 2019
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 3, 2019
@vagababov
Copy link
Copy Markdown
Contributor

vagababov commented Jul 3, 2019

/lgtm

/assign @evankanderson

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 3, 2019
evankanderson
evankanderson reviewed Jul 4, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

A few small notes and maybe one bug.

/approve

/hold
in case I caught a real bug. All the comments beyond the first are nits that you can ignore if you want.

Once you've addressed (Works As Intended or pushed another commit), feel free to remove the hold.

Comment thread metrics/metricstest/metricstest.go Outdated
Comment thread metrics/metricstest/metricstest.go Outdated
Comment thread metrics/metricstest/metricstest.go Outdated
Comment thread metrics/metricstest/metricstest.go Outdated
Comment thread metrics/metricstest/metricstest.go Outdated
Comment thread metrics/metricstest/metricstest.go Outdated
Comment thread metrics/metricstest/metricstest.go Outdated
Comment thread webhook/stats_reporter.go Outdated
@knative-prow-robot knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 4, 2019
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2019
@evankanderson
Copy link
Copy Markdown
Member

evankanderson commented Jul 8, 2019

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2019
@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented Jul 8, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anniefu, evankanderson

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

@evankanderson
Copy link
Copy Markdown
Member

evankanderson commented Jul 8, 2019

/hold cancel

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 8, 2019
@knative-prow-robot knative-prow-robot merged commit 84d3910 into knative:master Jul 8, 2019
@n3wscott
Copy link
Copy Markdown
Contributor

n3wscott commented Jul 9, 2019

This is causing NPE in eventing.

n3wscott
n3wscott reviewed Jul 9, 2019
View reviewed changes
Comment thread webhook/webhook.go
}

// Only report valid requests
ac.StatsReporter.ReportRequest(review.Request, response.Response, time.Since(ttStart))
Copy link
Copy Markdown
Contributor

@n3wscott n3wscott Jul 9, 2019

Choose a reason for hiding this comment

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

This causes a NPE. StatsReporter is new and nil in all repos.

Copy link
Copy Markdown
Contributor Author

@anniefu anniefu Jul 9, 2019

Choose a reason for hiding this comment

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

Ah yes, that makes sense. I was pulling this in and making the changes in the serving repo to initialize this correctly, but I forgot this was a common package :/

I'll push a fix out in a couple of hours or roll it back.

@n3wscott n3wscott mentioned this pull request Jul 9, 2019
Closed
@anniefu anniefu mentioned this pull request Jul 11, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@evankanderson evankanderson evankanderson left review comments

@grantr grantr Awaiting requested review from grantr

@mdemirhan mdemirhan Awaiting requested review from mdemirhan

+3 more reviewers

@vagababov vagababov vagababov left review comments

@n3wscott n3wscott n3wscott left review comments

@mattmoor-sockpuppet mattmoor-sockpuppet mattmoor-sockpuppet left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@vagababov vagababov

@evankanderson evankanderson

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@anniefu @yt3liu @vagababov @evankanderson @knative-prow-robot @n3wscott @grantr @mattmoor-sockpuppet @googlebot