You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2021/04/21 13:34:46 UTC
[GitHub] [incubator-yunikorn-core] kingamarton opened a new pull request #267: [YUNIKORN-645] Metrics endpoint doesn't export any metrics
kingamarton opened a new pull request #267:
URL: https://github.com/apache/incubator-yunikorn-core/pull/267
### What is this PR for?
A few sentences describing the overall goals of the pull request's commits.
First time? Check out the contributing guide - http://yunikorn.apache.org/community/how_to_contribute
After YUNIKORN-418 we are using a custom http handler, what while reading the response checks if the response status is changed. If yes, the further reading was skipped. When accessing the metrics endpoint, the response is sent is multiple sub parts. Since after the first one the status will be changed, the processing of the response is skipped.
With this PR I extended the check to skip the further processing if the status is changed and it is not 200-OK.
### What type of PR is it?
* [X] - Bug Fix
* [ ] - Improvement
* [ ] - Feature
* [ ] - Documentation
* [ ] - Hot Fix
* [ ] - Refactoring
### Todos
* [ ] - Task
### What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-645
### Screenshots (if appropriate)
### Questions:
* [ ] - The licenses files need update.
* [ ] - There is breaking changes for older versions.
* [ ] - It needs documentation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] kingamarton commented on pull request #267: [YUNIKORN-645] Metrics endpoint doesn't export any metrics
Posted by GitBox <gi...@apache.org>.
kingamarton commented on pull request #267:
URL: https://github.com/apache/incubator-yunikorn-core/pull/267#issuecomment-824084443
@manirajv06 can you please check this fix?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] manirajv06 commented on pull request #267: [YUNIKORN-645] Metrics endpoint doesn't export any metrics
Posted by GitBox <gi...@apache.org>.
manirajv06 commented on pull request #267:
URL: https://github.com/apache/incubator-yunikorn-core/pull/267#issuecomment-826544899
Idea behind using wrapper is to ensure all REST API's starts sending even JSON formatted error responses in single go instead of doing this in piece meal manner (i.e API wise). In later approach, We have to make changes for each API individually and requires co-ordination with UI side as well for each API. Where as, earlier approach of using wrapper to send JSON formatted error responses requires minimal work and co-ordination on both sides.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] manirajv06 commented on pull request #267: [YUNIKORN-645] Metrics endpoint doesn't export any metrics
Posted by GitBox <gi...@apache.org>.
manirajv06 commented on pull request #267:
URL: https://github.com/apache/incubator-yunikorn-core/pull/267#issuecomment-826281131
> Beside the issue that was mentioned by @yangwwei for the other _successful_ status codes I can also see an issue around error handling and non success codes. If we set `http.StatusInternalServerError` because we have encountered a problem when writing the body via the json encoder, we try to redo that same thing again. Similar for any error in the 400 range.
Intent was to convey the message to the client in case of any issues in sending json formatted error response. To avoid the cyclic dependency, was trying to send the message in plain text format with `http.StatusInternalServerError` because client depends on status code for first level checks before even trying to interpret the actual message (response body).
Now, logging only as ERROR and onus comes on the Infra admin to understand the reasons as part of general debugging. Thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] manirajv06 commented on a change in pull request #267: [YUNIKORN-645] Metrics endpoint doesn't export any metrics
Posted by GitBox <gi...@apache.org>.
manirajv06 commented on a change in pull request #267:
URL: https://github.com/apache/incubator-yunikorn-core/pull/267#discussion_r619772403
##########
File path: pkg/webservice/webservice.go
##########
@@ -81,7 +81,7 @@ type YResponseWriter struct {
func (rw *YResponseWriter) Write(bytes []byte) (int, error) {
rw.body = bytes
- if rw.statusCode == -1 {
+ if rw.statusCode == -1 || rw.statusCode == http.StatusOK {
Review comment:
Yes, correct. We should have taken a generic approach. Ensuring status code falls with in min and max success code seems to be clean one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] wilfred-s commented on pull request #267: [YUNIKORN-645] Metrics endpoint doesn't export any metrics
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on pull request #267:
URL: https://github.com/apache/incubator-yunikorn-core/pull/267#issuecomment-826481536
I understand what the intent was but there are a number of issues that show up.
The code must be able to handle _any_ HTTP response code. We also need to be able to handle 3rd party pieces like prometheus or other endpoints that will go through in the future. We have full control over our code and instead of an indirect update we can directly push the json formatted error we need into the message. There would not be any wrapping needed.
That is in line with the current way we return a custom json object in the `validateConf()`, be it with a `dao.ValidateConfResponse` object. The same can be used for errors too (using `w.WriteHeader()` with the appropriate http status code).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] wilfred-s commented on pull request #267: [YUNIKORN-645] Metrics endpoint doesn't export any metrics
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on pull request #267:
URL: https://github.com/apache/incubator-yunikorn-core/pull/267#issuecomment-825313229
I think we need to wind back the introduction of the `YResponseWriter` If there are calls that need specific handling of the errors then that should be part of the defined `HandlerFunc` which we specify in the web routes. We can still use a predefined error mechanism to get the details.
Beside the issue that was mentioned by @yangwwei for the other _successful_ status codes I can also see an issue around error handling and non success codes. If we set `http.StatusInternalServerError` because we have encountered a problem when writing the body via the json encoder, we try to redo that same thing again. Similar for any error in the 400 range.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] wilfred-s closed pull request #267: [YUNIKORN-645] Metrics endpoint doesn't export any metrics
Posted by GitBox <gi...@apache.org>.
wilfred-s closed pull request #267:
URL: https://github.com/apache/incubator-yunikorn-core/pull/267
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] manirajv06 commented on pull request #267: [YUNIKORN-645] Metrics endpoint doesn't export any metrics
Posted by GitBox <gi...@apache.org>.
manirajv06 commented on pull request #267:
URL: https://github.com/apache/incubator-yunikorn-core/pull/267#issuecomment-826281131
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] manirajv06 commented on a change in pull request #267: [YUNIKORN-645] Metrics endpoint doesn't export any metrics
Posted by GitBox <gi...@apache.org>.
manirajv06 commented on a change in pull request #267:
URL: https://github.com/apache/incubator-yunikorn-core/pull/267#discussion_r619772403
##########
File path: pkg/webservice/webservice.go
##########
@@ -81,7 +81,7 @@ type YResponseWriter struct {
func (rw *YResponseWriter) Write(bytes []byte) (int, error) {
rw.body = bytes
- if rw.statusCode == -1 {
+ if rw.statusCode == -1 || rw.statusCode == http.StatusOK {
Review comment:
Yes, correct. We should have taken a generic approach. Taken care now.
##########
File path: pkg/webservice/webservice.go
##########
@@ -81,7 +81,7 @@ type YResponseWriter struct {
func (rw *YResponseWriter) Write(bytes []byte) (int, error) {
rw.body = bytes
- if rw.statusCode == -1 {
+ if rw.statusCode == -1 || rw.statusCode == http.StatusOK {
Review comment:
Yes, correct. We should have taken a generic approach. Ensuring status code falls with in min and max success code seems to be clean one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] wilfred-s commented on pull request #267: [YUNIKORN-645] Metrics endpoint doesn't export any metrics
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on pull request #267:
URL: https://github.com/apache/incubator-yunikorn-core/pull/267#issuecomment-826481536
I understand what the intent was but there are a number of issues that show up.
The code must be able to handle _any_ HTTP response code. We also need to be able to handle 3rd party pieces like prometheus or other endpoints that will go through in the future. We have full control over our code and instead of an indirect update we can directly push the json formatted error we need into the message. There would not be any wrapping needed.
That is in line with the current way we return a custom json object in the `validateConf()`, be it with a `dao.ValidateConfResponse` object. The same can be used for errors too (using `w.WriteHeader()` with the appropriate http status code).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull request #267: [YUNIKORN-645] Metrics endpoint doesn't export any metrics
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #267:
URL: https://github.com/apache/incubator-yunikorn-core/pull/267#discussion_r618826128
##########
File path: pkg/webservice/webservice.go
##########
@@ -81,7 +81,7 @@ type YResponseWriter struct {
func (rw *YResponseWriter) Write(bytes []byte) (int, error) {
rw.body = bytes
- if rw.statusCode == -1 {
+ if rw.statusCode == -1 || rw.statusCode == http.StatusOK {
Review comment:
This seems to fix the problem we saw here, but this line could still cause issues.
For example, what if we get some other HTTP code defined in https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html. Such as `Accepted`, `Created`, etc. This will still skip writing the body to the client-side, which is dangerous.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] manirajv06 commented on a change in pull request #267: [YUNIKORN-645] Metrics endpoint doesn't export any metrics
Posted by GitBox <gi...@apache.org>.
manirajv06 commented on a change in pull request #267:
URL: https://github.com/apache/incubator-yunikorn-core/pull/267#discussion_r619772403
##########
File path: pkg/webservice/webservice.go
##########
@@ -81,7 +81,7 @@ type YResponseWriter struct {
func (rw *YResponseWriter) Write(bytes []byte) (int, error) {
rw.body = bytes
- if rw.statusCode == -1 {
+ if rw.statusCode == -1 || rw.statusCode == http.StatusOK {
Review comment:
Yes, correct. We should have taken a generic approach. Taken care now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] codecov[bot] commented on pull request #267: [YUNIKORN-645] Metrics endpoint doesn't export any metrics
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #267:
URL: https://github.com/apache/incubator-yunikorn-core/pull/267#issuecomment-824081544
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/267?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#267](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/267?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fa2df05) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5a1c19e) will **increase** coverage by `1.74%`.
> The diff coverage is `69.60%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/267?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #267 +/- ##
==========================================
+ Coverage 63.46% 65.21% +1.74%
==========================================
Files 60 60
Lines 5220 5563 +343
==========================================
+ Hits 3313 3628 +315
- Misses 1747 1760 +13
- Partials 160 175 +15
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/267?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/267/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `5.78% <0.00%> (-0.49%)` | :arrow_down: |
| [pkg/scheduler/nodes\_usage\_monitor.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/267/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9ub2Rlc191c2FnZV9tb25pdG9yLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/267/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
| [pkg/scheduler/partition\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/267/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb25fbWFuYWdlci5nbw==) | `20.00% <50.00%> (ø)` | |
| [pkg/scheduler/objects/queue.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/267/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL3F1ZXVlLmdv) | `69.18% <58.33%> (-0.27%)` | :arrow_down: |
| [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/267/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `56.58% <64.70%> (+7.67%)` | :arrow_up: |
| [pkg/scheduler/objects/node.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/267/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL25vZGUuZ28=) | `81.98% <75.00%> (+0.16%)` | :arrow_up: |
| [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/267/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `44.44% <75.00%> (+31.54%)` | :arrow_up: |
| [pkg/scheduler/partition.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/267/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb24uZ28=) | `72.53% <83.05%> (+6.49%)` | :arrow_up: |
| [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/267/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `58.82% <87.50%> (+4.68%)` | :arrow_up: |
| ... and [8 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/267/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/267?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/267?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [4cef5d9...fa2df05](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/267?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] wilfred-s commented on pull request #267: [YUNIKORN-645] Metrics endpoint doesn't export any metrics
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on pull request #267:
URL: https://github.com/apache/incubator-yunikorn-core/pull/267#issuecomment-830981036
This was fixed via #268. It removes the wrapping response handler.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org