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

Add requestLogHandler to user container as well as health check #12229

Merged
merged 1 commit into from
Nov 8, 2021
Merged

Add requestLogHandler to user container as well as health check #12229

merged 1 commit into from
Nov 8, 2021

Conversation

nader-ziada
Copy link
Member

@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 5, 2021
@knative-prow-robot knative-prow-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. area/autoscale area/networking labels Nov 5, 2021
@codecov
Copy link

codecov bot commented Nov 5, 2021 •
edited
Loading

Codecov Report

Merging #12229 (06e38f8) into main (525a4cb) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #12229      +/-   ##
==========================================
- Coverage   87.32%   87.30%   -0.02%     
==========================================
  Files         195      195              
  Lines        9569     9571       +2     
==========================================
  Hits         8356     8356              
- Misses        934      936       +2     
  Partials      279      279
Impacted Files Coverage Δ
cmd/queue/main.go 0.49% <0.00%> (-0.01%) ⬇️

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 525a4cb...06e38f8. Read the comment docs.

julz
julz approved these changes Nov 8, 2021
View reviewed changes
Copy link
Member

@julz julz left a comment

Choose a reason for hiding this comment

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

do we know why the e2e test isn't catching this? It'd be great to fix the coverage there

/lgtm
/approve
/hold in case you want to move this down in to the existing if block

cmd/queue/main.go
@@ -311,9 +311,14 @@ func buildServer(ctx context.Context, env config, drainer *pkghandler.Drainer, p
if tracingEnabled {
composedHandler = tracing.HTTPSpanMiddleware(composedHandler)
}
if env.ServingEnableRequestLog {
composedHandler = requestLogHandler(logger, composedHandler, env)
Copy link
Member

Choose a reason for hiding this comment

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

I'd be tempted to move this down in to the other if env.ServingEnableRequestLog block since it's only 3 lines down from here. I'm OK to leave it here if you disagree, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

my point was to keep each handler in its own block of code

Copy link
Member

@julz julz Nov 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

👍 fine with me, I don't feel super strongly

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 8, 2021
@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2021
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: julz, 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-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2021
@julz
Copy link
Member

julz commented Nov 8, 2021

/unhold

@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 Nov 8, 2021
@knative-prow-robot knative-prow-robot merged commit fb6a3ab into knative:main Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julz julz julz approved these changes

@dprotaso dprotaso Awaiting requested review from dprotaso

Assignees

@julz julz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/autoscale area/networking cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nader-ziada @knative-prow-robot @julz