{"action":"created","issue":{"url":"https://api.github.com/repos/grpc/grpc/issues/15196","repository_url":"https://api.github.com/repos/grpc/grpc","labels_url":"https://api.github.com/repos/grpc/grpc/issues/15196/labels{/name}","comments_url":"https://api.github.com/repos/grpc/grpc/issues/15196/comments","events_url":"https://api.github.com/repos/grpc/grpc/issues/15196/events","html_url":"https://github.com/grpc/grpc/pull/15196","id":318141552,"number":15196,"title":"Add load reporter for server load reporting","user":{"login":"AspirinSJL","id":3314176,"avatar_url":"https://avatars0.githubusercontent.com/u/3314176?v=4","gravatar_id":"","url":"https://api.github.com/users/AspirinSJL","html_url":"https://github.com/AspirinSJL","followers_url":"https://api.github.com/users/AspirinSJL/followers","following_url":"https://api.github.com/users/AspirinSJL/following{/other_user}","gists_url":"https://api.github.com/users/AspirinSJL/gists{/gist_id}","starred_url":"https://api.github.com/users/AspirinSJL/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/AspirinSJL/subscriptions","organizations_url":"https://api.github.com/users/AspirinSJL/orgs","repos_url":"https://api.github.com/users/AspirinSJL/repos","events_url":"https://api.github.com/users/AspirinSJL/events{/privacy}","received_events_url":"https://api.github.com/users/AspirinSJL/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":40,"created_at":"2018-04-26T18:14:13Z","updated_at":"2018-05-29T20:55:11Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/grpc/grpc/pulls/15196","html_url":"https://github.com/grpc/grpc/pull/15196","diff_url":"https://github.com/grpc/grpc/pull/15196.diff","patch_url":"https://github.com/grpc/grpc/pull/15196.patch"},"body":"This is a continuation of #14793 ~and currently based on that PR~.\r\n\r\n~This is not yet ready for review. I hope to see some quick feedback from @isturdy on my usage of the opencensus APIs.~\r\n\r\nThis is ready for review except for the `get_cpu_stats()` implementation on the platforms other than Linux.\r\n\r\nI skipped some low-risk but detailed tests for now.\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/grpc/grpc/15196)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/grpc/grpc/issues/comments/392941251","html_url":"https://github.com/grpc/grpc/pull/15196#issuecomment-392941251","issue_url":"https://api.github.com/repos/grpc/grpc/issues/15196","id":392941251,"user":{"login":"AspirinSJL","id":3314176,"avatar_url":"https://avatars0.githubusercontent.com/u/3314176?v=4","gravatar_id":"","url":"https://api.github.com/users/AspirinSJL","html_url":"https://github.com/AspirinSJL","followers_url":"https://api.github.com/users/AspirinSJL/followers","following_url":"https://api.github.com/users/AspirinSJL/following{/other_user}","gists_url":"https://api.github.com/users/AspirinSJL/gists{/gist_id}","starred_url":"https://api.github.com/users/AspirinSJL/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/AspirinSJL/subscriptions","organizations_url":"https://api.github.com/users/AspirinSJL/orgs","repos_url":"https://api.github.com/users/AspirinSJL/repos","events_url":"https://api.github.com/users/AspirinSJL/events{/privacy}","received_events_url":"https://api.github.com/users/AspirinSJL/received_events","type":"User","site_admin":false},"created_at":"2018-05-29T20:55:11Z","updated_at":"2018-05-29T20:55:11Z","author_association":"CONTRIBUTOR","body":"Thanks for reviewing!\n\n---\n\nReview status: 19 of 23 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.\n\n---\n\n*[build.yaml, line 1894 at r1](https://reviewable.io/reviews/grpc/grpc/15196#-LDgQNLw6L8duT3SQji1:-LDgoZgq3cSUJm-kRUsa:b1j3mgf) ([raw file](https://github.com/grpc/grpc/blob/68342ca6d74da6c3b10427fb7b1f32dcad3830f3/build.yaml#L1894)):*\n<details><summary><i>Previously, markdroth (Mark D. Roth) wrote…</i></summary><blockquote>\n\nI thought we decided that this won't build via make?\n</blockquote></details>\n\nI keep this for now because it seems that we still need some discussion about this? See https://github.com/grpc/grpc/pull/15349#discussion_r188754176\n\n---\n\n*[src/cpp/server/load_reporter/get_cpu_stats.h, line 29 at r1](https://reviewable.io/reviews/grpc/grpc/15196#-LDgR0CO71OleigDA8rA:-LDgp_ufD_Q_g3kuuF5H:b-896fix) ([raw file](https://github.com/grpc/grpc/blob/68342ca6d74da6c3b10427fb7b1f32dcad3830f3/src/cpp/server/load_reporter/get_cpu_stats.h#L29)):*\n<details><summary><i>Previously, markdroth (Mark D. Roth) wrote…</i></summary><blockquote>\n\nThis function name should be C++-style instead of C style: `GetCpuStats()`.\n</blockquote></details>\n\nDone.\n\n---\n\n*[src/cpp/server/load_reporter/load_data_store.h, line 78 at r1](https://reviewable.io/reviews/grpc/grpc/15196#-LDgRluq3mEZ4GYXX_6q:-LDgr5qwEZnhfQXm44uj:b-896fix) ([raw file](https://github.com/grpc/grpc/blob/68342ca6d74da6c3b10427fb7b1f32dcad3830f3/src/cpp/server/load_reporter/load_data_store.h#L78)):*\n<details><summary><i>Previously, markdroth (Mark D. Roth) wrote…</i></summary><blockquote>\n\nNo need for `explicit` here, since there are 2 parameters.\n\nActually, it looks like we're unnecessarily using `explicit` even for some of the existing ctors.  Please remove throughout, except in cases where the ctor has a single parameter.\n</blockquote></details>\n\nDone.\n\n---\n\n*[src/cpp/server/load_reporter/load_data_store.h, line 127 at r1](https://reviewable.io/reviews/grpc/grpc/15196#-LDgRvdn7d9qohcv6Km1:-LDgrB3Q1-T5-lhUhTzC:begsem3) ([raw file](https://github.com/grpc/grpc/blob/68342ca6d74da6c3b10427fb7b1f32dcad3830f3/src/cpp/server/load_reporter/load_data_store.h#L127)):*\n<details><summary><i>Previously, markdroth (Mark D. Roth) wrote…</i></summary><blockquote>\n\nWhy are these parameters changing from int64 to double?\n\nFor that matter, why are some of the parameters doubles even before this PR?  Shouldn't these all be integers?  It's not clear to me why we would have fractional values for any of these.\n</blockquote></details>\n\nLogically they should be integers. But there is some tricky problem with OpenCensus here: when making testing view data, OpenCensus will treat the input values as separate measurements and add them together according to the data type (i.e., if it's int, add 1; if it's double, add the original input value). For the testing to be effective and the code length to be reasonable, I changed the data type to double.\n\nBut after upon thought, I should actually make a feature request or maybe a pull request to OpenCensus. @isturdy Does it sound reasonable to add a method to set the final view data values directly?\n\n---\n\n*[src/cpp/server/load_reporter/load_data_store.cc, line 81 at r1](https://reviewable.io/reviews/grpc/grpc/15196#-LDgTC_--TOe7oTumi5c:-LDgx-N18C9wEuSPo2tn:b62d3t2) ([raw file](https://github.com/grpc/grpc/blob/68342ca6d74da6c3b10427fb7b1f32dcad3830f3/src/cpp/server/load_reporter/load_data_store.cc#L81)):*\n<details><summary><i>Previously, markdroth (Mark D. Roth) wrote…</i></summary><blockquote>\n\nI would avoid using `std::stoi()`, since it can generate exceptions, which should not be used in Google C++ code, as per the style guide.\n\nSince this code imposes a transitive dependency on absl anyway, consider using `absl::SimpleAtoi()` instead.\n</blockquote></details>\n\nOh, good to know that.\n\nChanged to `sscanf()` to be consistent with a change below.\n\n---\n\n*[src/cpp/server/load_reporter/load_data_store.cc, line 99 at r1](https://reviewable.io/reviews/grpc/grpc/15196#-LDgVkPz8Q4-AZBaHo9x:-LDh1MWo9bKOAmdZWUF4:b-ohjkaf) ([raw file](https://github.com/grpc/grpc/blob/68342ca6d74da6c3b10427fb7b1f32dcad3830f3/src/cpp/server/load_reporter/load_data_store.cc#L99)):*\n<details><summary><i>Previously, markdroth (Mark D. Roth) wrote…</i></summary><blockquote>\n\nSimilarly, suggest using `absl::SimpleAtoi()` instead of `std::stoul()`.  Same thing throughout this function.\n</blockquote></details>\n\nDone.\n\nSince `absl::SimpleAtoi()` only supports base of 10, I changed to `sscanf()`\n\n---\n\n*[src/cpp/server/load_reporter/load_reporter.h, line 67 at r1](https://reviewable.io/reviews/grpc/grpc/15196#-LDgXaBe9foQGa6xhg6m:-LDh45tl7ecLnhunvhne:btdsfzr) ([raw file](https://github.com/grpc/grpc/blob/68342ca6d74da6c3b10427fb7b1f32dcad3830f3/src/cpp/server/load_reporter/load_reporter.h#L67)):*\n<details><summary><i>Previously, markdroth (Mark D. Roth) wrote…</i></summary><blockquote>\n\nData members should always be private, not protected.\n\nhttps://google.github.io/styleguide/cppguide.html#Access_Control\n</blockquote></details>\n\nI see.\n\n---\n\n*[src/cpp/server/load_reporter/load_reporter.cc, line 212 at r1](https://reviewable.io/reviews/grpc/grpc/15196#-LDgZ4OY81qjKYDhhsNn:-LDhWx1v2wUumH23Nyj0:b-scttgq) ([raw file](https://github.com/grpc/grpc/blob/68342ca6d74da6c3b10427fb7b1f32dcad3830f3/src/cpp/server/load_reporter/load_reporter.cc#L212)):*\n<details><summary><i>Previously, markdroth (Mark D. Roth) wrote…</i></summary><blockquote>\n\nConsider using `absl::StrFormat()`.\n</blockquote></details>\n\nUnfortunately, I can't find str_format.h in our submodule. I guess it's a version problem?\n\nAnyways, I changed to `sprintf()`, which seems similar to `absl::StrFormat()`.\n\n---\n\n*[src/cpp/server/load_reporter/util.h, line 25 at r1](https://reviewable.io/reviews/grpc/grpc/15196#-LDgSdSw97U3AImOop_6:-LDhXr3vDMuZHWUZVnqS:bs7wumb) ([raw file](https://github.com/grpc/grpc/blob/68342ca6d74da6c3b10427fb7b1f32dcad3830f3/src/cpp/server/load_reporter/util.h#L25)):*\n<details><summary><i>Previously, markdroth (Mark D. Roth) wrote…</i></summary><blockquote>\n\nSince all that's in this file is constants, how about renaming it from util.h to constants.h?\n</blockquote></details>\n\nDone.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io/reviews/grpc/grpc/15196#-:-LDgo2QPAfMXrhXNSUh1:b-yz40g0)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"AspirinSJL"}}