Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove Prometheus stat reporter from queue-proxy #12961

Merged
merged 3 commits into from
Jul 11, 2022
Merged

Remove Prometheus stat reporter from queue-proxy #12961

merged 3 commits into from
Jul 11, 2022

Conversation

nader-ziada
Copy link
Member

Fixes #11126

Proposed Changes

  • remove internal Prometheus stat reporter from queue-proxy since autoscaler uses the protobuf

Release Note

Remove internal Prometheus stat reporter from queue-proxy

@knative-prow knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/autoscale area/networking labels May 20, 2022
@codecov
Copy link

codecov bot commented May 20, 2022 •
edited
Loading

Codecov Report

Merging #12961 (ddb9408) into main (6ceb219) will increase coverage by 0.02%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##             main   #12961      +/-   ##
==========================================
+ Coverage   86.76%   86.79%   +0.02%     
==========================================
  Files         197      196       -1     
  Lines       14485    14418      -67     
==========================================
- Hits        12568    12514      -54     
+ Misses       1621     1609      -12     
+ Partials      296      295       -1
Impacted Files Coverage Δ
cmd/queue/main.go 0.66% <0.00%> (+0.01%) ⬆️
pkg/queue/stats_handler.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ceb219...ddb9408. Read the comment docs.

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2022
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 22, 2022
@knative-prow knative-prow bot added the area/API API objects and controllers label May 22, 2022
@nader-ziada
Copy link
Member Author

/retest

@nader-ziada
Copy link
Member Author

Hello @skonto, do you have a concern about removing the Prometheus reporter from queue-proxy? you had some comments on the related issue

@nader-ziada nader-ziada changed the title [WIP] Remove Prometheus stat reporter from queue-proxy Remove Prometheus stat reporter from queue-proxy May 25, 2022
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 25, 2022
@dprotaso
Copy link
Member

/hold for comment

also might best wait to land this change until after 1.5 - since we can mention this in the release notes/mailing list

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 25, 2022
@nader-ziada
Copy link
Member Author

yes, better to wait after release

@dprotaso dprotaso added this to the v1.6.0 milestone May 25, 2022
@psschwei
Copy link
Contributor

+1 for me

@skonto
Copy link
Contributor

skonto commented Jun 3, 2022

One question that I think was asked before is whether this could be useful for quick debugging at the queue proxy side, especially for contributors?

dprotaso
dprotaso reviewed Jun 7, 2022
View reviewed changes
cmd/queue/main.go
@@ -139,14 +139,6 @@ func main() {
// Report stats on Go memory usage every 30 seconds.
metrics.MemStatsOrDie(ctx)

// Setup reporters and processes to handle stat reporting.
promStatReporter, err := queue.NewPrometheusStatsReporter(
Copy link
Member

Choose a reason for hiding this comment

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

I think we can drop this reporter from the code base

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

@dprotaso
Copy link
Member

dprotaso commented Jun 7, 2022

One question that I think was asked before is whether this could be useful for quick debugging at the queue proxy side, especially for contributors?

If this is the case then I think we could simply just json marshal the stat vs. having a full fledge prometheus endpoint.

We also have user facing metrics - https://knative.dev/docs/serving/services/service-metrics/

@dprotaso
Copy link
Member

dprotaso commented Jun 7, 2022

/hold cancel since the release is passed

@psschwei
Copy link
Contributor

@nader-ziada did we let enough time pass for comment on whether to go forward removing this?

@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 4, 2022
@nader-ziada
Copy link
Member Author

@nader-ziada did we let enough time pass for comment on whether to go forward removing this?

I think so

@nader-ziada
Copy link
Member Author

/retest

@nader-ziada
Copy link
Member Author

@psschwei do you think this can make the release?

@psschwei
Copy link
Contributor

psschwei commented Jul 6, 2022

I think it should be fine to merge this. Probably worthwhile to send a note to knative-dev announcing that we're removing it, then we can merge later this week.

@nader-ziada
Copy link
Member Author

I think it should be fine to merge this. Probably worthwhile to send a note to knative-dev announcing that we're removing it, then we can merge later this week.

sent email to [email protected] with subject Prometheus stat reporter will be removed from queue-proxy in coming serving release on July 6th 2022

@dprotaso
Copy link
Member

dprotaso commented Jul 11, 2022 •
edited
Loading

Emails are out and there aren't any replies
/hold cancel

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 11, 2022
@dprotaso
Copy link
Member

/lgtm
/approve

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2022
@knative-prow
Copy link

knative-prow bot commented Jul 11, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, nader-ziada

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

@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 11, 2022
@knative-prow knative-prow bot merged commit 3360e41 into knative:main Jul 11, 2022
@tikr7 tikr7 mentioned this pull request May 31, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dprotaso dprotaso dprotaso left review comments

Assignees

@dprotaso dprotaso

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers area/autoscale area/networking 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
v1.6.0
Development

Successfully merging this pull request may close these issues.

Drop prometheus stat reporter from QP
5 participants
@nader-ziada @dprotaso @psschwei @skonto @knative-prow-robot