Skip to content
This repository has been archived by the owner on Dec 15, 2021. It is now read-only.

Fixed the issue of metrics for golang runtime #1126

Merged
merged 6 commits into from Apr 6, 2020

Conversation

Shashankft9
Copy link
Contributor

@Shashankft9 Shashankft9 commented Apr 2, 2020

Issue Ref: #1020

Description: Removed promhttp.Handler from kubeless/runtime/stable/golang/kubeless.tpl.go and put it inside the pkg/function-proxy/utils/proxy-utils.go so that it can scrape the registered metrics.

This should make it possible to get the registered metrics by doing a simple curl on the http-function-port/metrics

TODOs:

  • Ready to review
  • Automated Tests
  • Docs

@Shashankft9 Shashankft9 changed the title Metrics Fixed the issue of metrics for golang runtime Apr 2, 2020
Copy link
Contributor

@andresmgot andresmgot 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 PR! See my comment.

Also, you have some lint errors:

./script/validate-lint
Errors from golint:
pkg/function-proxy/utils/proxy-utils.go:40:2: exported var FuncHistogram should have comment or be unexported
pkg/function-proxy/utils/proxy-utils.go:54:1: exported function PromHTTPHandler should have comment or be unexported

Please fix the above errors. You can test via "golint" and commit the result.

Comment on lines 54 to 56
func PromHTTPHandler() http.Handler{
return promhttp.Handler()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct me if I am wrong but I think this is the only needed change. You can still use prometheus instead than promauto and avoid exposing FuncHistogram, FuncCalls and FuncErrors so the only change in this PR would be exposing this function. Can you try that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well I opted for promauto anyway because if we are to add more metrics in future we wont be needed/bothered to register them all, wherever in whichever function we define them, they will be registered automatically. It makes this metrics part a bit easier.
And FuncHistogram instead of funcHistogram for if we want to increment them in some other packages in future.
Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

but those are different topics, the goal of this PR is to fix the current metrics so I would leave it as minimal as possible to avoid confusion in the future of what did the trick. Then we can open a different PR to modify/add more metrics if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, we can take this up later.

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

Can you also remove the changes in go.mod, go.sum and vendor? I think they are not needed either.

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

Great thanks!

@andresmgot andresmgot merged commit 2644ad5 into vmware-archive:master Apr 6, 2020
@Shashankft9
Copy link
Contributor Author

anyone looking for the fix can apply this in their cluster in the meantime while there is no new release:
https://2824-73902337-gh.circle-artifacts.com/0/build-manifests/kubeless.yaml and restart the kubeless-controller-manager pod.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants