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.

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

feat: register the metrics with manager #220

Merged
merged 1 commit into from
Jun 29, 2022

Conversation

viveksyngh
Copy link
Contributor

No description provided.

@viveksyngh viveksyngh force-pushed the update-metrics branch 2 times, most recently from e35b09a to e3553d8 Compare June 24, 2022 07:28
Copy link
Member

@prometherion prometherion left a comment

Choose a reason for hiding this comment

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

Let's address the linters complains, and let's add // nolint:gochecknoinits and // nolint:gochecknoglobals.

@prometherion
Copy link
Member

I know this is out of the scope of this PR, however, I got a question since you're the author of the following:
https://github.com/clastix/capsule-proxy/blob/8ed733e4b0100879be30570b0eacc45904fdb300/internal/webserver/middleware/metrics.go#L33

Why are we hard-coding the status code to be 200? This could be misleading since if I'm unauthenticated, I'm expecting 403.

curl localhost:8001/api/v1/namespaces -v                                                                                                                 (kind-dataprotector/default)
*   Trying 127.0.0.1:8001...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8001 (#0)
> GET /api/v1/namespaces HTTP/1.1
> Host: localhost:8001
> User-Agent: curl/7.68.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 403 Forbidden
< Cache-Control: no-cache, private
< Content-Length: 309
< Content-Type: application/json
< Date: Mon, 27 Jun 2022 16:11:25 GMT
< X-Content-Type-Options: nosniff
< X-Kubernetes-Pf-Flowschema-Uid: 825c0a5c-a119-4db2-bb6a-36ca1e73cc99
< X-Kubernetes-Pf-Prioritylevel-Uid: 2d66fcb3-6520-4240-9b72-8d8a7c516b13
< 
{
  "kind": "Status",
  "apiVersion": "v1",
  "metadata": {
    
  },
  "status": "Failure",
  "message": "namespaces is forbidden: User \"alice\" cannot list resource \"namespaces\" in API group \"\" at the cluster scope",
  "reason": "Forbidden",
  "details": {
    "kind": "namespaces"
  },
  "code": 403
* Connection #0 to host localhost left intact
}

Just a silly example, it could be any other error.

@viveksyngh
Copy link
Contributor Author

@prometherion The status code is not getting hardcoded and it will always be the same as the status code returned handler.

we are creating a response writer with the below statement

https://github.com/clastix/capsule-proxy/blob/8ed733e4b0100879be30570b0eacc45904fdb300/internal/webserver/middleware/metrics.go#L57

and this response writer is getting passed to handlers in the chain which will be updating the status code to be same as what is returned by the handler
https://github.com/clastix/capsule-proxy/blob/8ed733e4b0100879be30570b0eacc45904fdb300/internal/webserver/middleware/metrics.go#L33

and we are passing the updated status code to Prometheus metrics labels, the 200 is put just to create a response writer with a valid HTTP status code.
https://github.com/clastix/capsule-proxy/blob/8ed733e4b0100879be30570b0eacc45904fdb300/internal/webserver/middleware/metrics.go#L60

Copy link
Member

@prometherion prometherion left a comment

Choose a reason for hiding this comment

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

Thanks for the kind explanation of this PR, LGTM, and astonishing work! 🚀

@prometherion prometherion added this to the v0.3.0 milestone Jun 29, 2022
@prometherion prometherion merged commit ae7df54 into projectcapsule:master Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants