Skip to content

Add activator metrics and dashboard#1726

Merged
josephburnett merged 4 commits intoknative:masterfrom
akyyy:activator-metrics3
Aug 2, 2018
Merged

Add activator metrics and dashboard#1726
josephburnett merged 4 commits intoknative:masterfrom
akyyy:activator-metrics3

Conversation

@akyyy
Copy link
Copy Markdown
Contributor

@akyyy akyyy commented Jul 26, 2018

Fixes #743

Proposed Changes

  • Add activator metrics
  • Add activator dashboard

@google-prow-robot google-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 26, 2018
@akyyy akyyy mentioned this pull request Jul 26, 2018
Closed
@akyyy
Copy link
Copy Markdown
Contributor Author

akyyy commented Jul 26, 2018 •
edited
Loading

Depending on how soon #1689 is finished, I may pick up issue #1727 in this pull request or later.

@akyyy
Copy link
Copy Markdown
Contributor Author

akyyy commented Jul 26, 2018

This pull request is moved from #1567 after I created a new repo. The old pr #1567 has all review comments.

mdemirhan
mdemirhan approved these changes Jul 31, 2018
View reviewed changes
Comment thread pkg/activator/dedupe.go Outdated

func (a *dedupingActivator) activate(id revisionID) {
endpoint, status, err := a.activator.ActiveEndpoint(id.namespace, id.name)
logger := loggerWithRevisionInfo(a.logger, id.namespace, id.name)
Copy link
Copy Markdown
Contributor

@mdemirhan mdemirhan Jul 31, 2018

Choose a reason for hiding this comment

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

Seems like an overkill to just report the serving state. Is this an important thing to report?

Copy link
Copy Markdown
Contributor Author

@akyyy akyyy Aug 1, 2018

Choose a reason for hiding this comment

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

I simplified the code to report the serving state.

@mdemirhan
Copy link
Copy Markdown
Contributor

mdemirhan commented Jul 31, 2018

/lgtm
/approve

I have a minor comment, but I don't think it should block this PR. If you agree, please make the changes in a separate PR. If you don't, then just disregard :)

@tanzeeb This change is likely going to be disruptive to your changes as well. I recommend talking to @akyyy to see how you can best incorporate these changes into your refactoring.

@google-prow-robot google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 31, 2018
@google-prow-robot google-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2018
@akyyy
Copy link
Copy Markdown
Contributor Author

akyyy commented Aug 1, 2018

/retest

josephburnett
josephburnett approved these changes Aug 1, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@josephburnett josephburnett left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@google-prow-robot google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2018
@josephburnett
Copy link
Copy Markdown
Contributor

josephburnett commented Aug 1, 2018

/assign @mattmoor

@google-prow-robot google-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2018
@akyyy
Copy link
Copy Markdown
Contributor Author

akyyy commented Aug 2, 2018

Gentle ping. @mattmoor @tcnghia Could you please take a look? I added configuration to the header in pkg/controller/names.go, so we could add configuration dimension to the metrics. Thanks!

@akyyy
Copy link
Copy Markdown
Contributor Author

akyyy commented Aug 2, 2018

/retest

1 similar comment
@akyyy
Copy link
Copy Markdown
Contributor Author

akyyy commented Aug 2, 2018

/retest

@knative-metrics-robot
Copy link
Copy Markdown

knative-metrics-robot commented Aug 2, 2018

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to run the coverage report again

File Old Coverage New Coverage Delta
pkg/activator/stats_reporter.go Do not exist 78.4%
pkg/activator/revision.go 80.0% 80.4% 0.4
pkg/controller/route/route.go 96.6% 96.6% 0.1

mattmoor
mattmoor reviewed Aug 2, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

/approve

For new header in pkg/controller

@google-prow-robot
Copy link
Copy Markdown

google-prow-robot commented Aug 2, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akyyy, josephburnett, mattmoor, mdemirhan

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

@google-prow-robot google-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2018
@josephburnett
Copy link
Copy Markdown
Contributor

josephburnett commented Aug 2, 2018

/mega
/lgtm

@google-prow-robot google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2018
@josephburnett josephburnett merged commit fb0e261 into knative:master Aug 2, 2018
@mattmoor
Copy link
Copy Markdown
Member

mattmoor commented Aug 2, 2018

@josephburnett Please let tide merge things.

@tcnghia tcnghia mentioned this pull request Oct 19, 2018
Merged
@akyyy akyyy deleted the activator-metrics3 branch November 28, 2018 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@evankanderson evankanderson Awaiting requested review from evankanderson

@grantr grantr Awaiting requested review from grantr

3 more reviewers

@mattmoor mattmoor mattmoor left review comments

@josephburnett josephburnett josephburnett approved these changes

@mdemirhan mdemirhan mdemirhan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@tcnghia tcnghia

@josephburnett josephburnett

@mattmoor mattmoor

@mdemirhan mdemirhan

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@akyyy @mdemirhan @josephburnett @knative-metrics-robot @google-prow-robot @mattmoor @tcnghia