{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/4584","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/4584/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/4584/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/4584/events","html_url":"https://github.com/cockroachdb/cockroach/pull/4584","id":135583465,"number":4584,"title":"Remove Event Feed","user":{"login":"mrtracy","id":6969858,"avatar_url":"https://avatars.githubusercontent.com/u/6969858?v=3","gravatar_id":"","url":"https://api.github.com/users/mrtracy","html_url":"https://github.com/mrtracy","followers_url":"https://api.github.com/users/mrtracy/followers","following_url":"https://api.github.com/users/mrtracy/following{/other_user}","gists_url":"https://api.github.com/users/mrtracy/gists{/gist_id}","starred_url":"https://api.github.com/users/mrtracy/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mrtracy/subscriptions","organizations_url":"https://api.github.com/users/mrtracy/orgs","repos_url":"https://api.github.com/users/mrtracy/repos","events_url":"https://api.github.com/users/mrtracy/events{/privacy}","received_events_url":"https://api.github.com/users/mrtracy/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2016-02-22T23:57:46Z","updated_at":"2016-02-23T00:48:49Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/4584","html_url":"https://github.com/cockroachdb/cockroach/pull/4584","diff_url":"https://github.com/cockroachdb/cockroach/pull/4584.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/4584.patch"},"body":"This commit removes the Event Feed from all cockroach systems. The feed was\r\npreviously being used to decouple the metric status system from lower-level\r\nsystems; however, `metric.Registry` has recently been created to perform that\r\ntask better. The new system is far more streamlined and easier to understand.\r\n\r\nIn the new factoring, each component in the server (e.g. `Store`, `SqlExecutor`)\r\ncreates a metric.Registry and directly adds metric counters to it. Each\r\ncomponent exposes this registry through the `Registry()` method. The server is\r\nresponsible for instantiating a `MetricsRecorder` object, and giving the\r\nrecorder a reference to all component registries. The recorder is responsible\r\nfor converting the metrics in each registry into time series data, status\r\n  summaries or JSON output.\r\n\r\nThere is a bit of \"ugly\" code present in the recorder to continue supporting the\r\ncurrent structure of the StoreStatusSummary object. However, #4465 has been\r\nlogged (and mentioned in several TODOs) to tackle that issue in the short term.\r\n\r\nCloses #4021\r\nCloses #1531 (by removing the need for the test)\r\n\r\nOpened #4583 to add additional testing in the short term.\n\n<!-- Reviewable:start -->\n[<img src=\"https://reviewable.io/review_button.svg\" height=\"40\" alt=\"Review on Reviewable\"/>](https://reviewable.io/reviews/cockroachdb/cockroach/4584)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/187456134","html_url":"https://github.com/cockroachdb/cockroach/pull/4584#issuecomment-187456134","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/4584","id":187456134,"user":{"login":"tamird","id":1535036,"avatar_url":"https://avatars.githubusercontent.com/u/1535036?v=3","gravatar_id":"","url":"https://api.github.com/users/tamird","html_url":"https://github.com/tamird","followers_url":"https://api.github.com/users/tamird/followers","following_url":"https://api.github.com/users/tamird/following{/other_user}","gists_url":"https://api.github.com/users/tamird/gists{/gist_id}","starred_url":"https://api.github.com/users/tamird/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tamird/subscriptions","organizations_url":"https://api.github.com/users/tamird/orgs","repos_url":"https://api.github.com/users/tamird/repos","events_url":"https://api.github.com/users/tamird/events{/privacy}","received_events_url":"https://api.github.com/users/tamird/received_events","type":"User","site_admin":false},"created_at":"2016-02-23T00:48:49Z","updated_at":"2016-02-23T00:48:49Z","body":"Science dog on most of this, someone else should take a look.\n\n---\n\nReviewed 22 of 22 files at r1.\nReview status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.\n\n---\n\n*[server/node.go, line 534 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/4584#-KBAWSNPiwSwNIS51AZ5:-KBAWSNQL1I-Dch2zn_d:-2019541756) ([raw file](https://github.com/cockroachdb/cockroach/blob/316ef65170f9b40b131616634b127abd1cd569a7/server/node.go#L534)):*\ndoes the loop only trigger publishing or does it also trigger calculation? i ask because the word \"periodically\" appears twice.\n\n---\n\n*[server/status/recorder.go, line 85 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/4584#-KBAWwAKde-EuBlMNvFc:-KBAWwAKde-EuBlMNvFd:368483270) ([raw file](https://github.com/cockroachdb/cockroach/blob/316ef65170f9b40b131616634b127abd1cd569a7/server/status/recorder.go#L85)):*\nmake an anonymous struct:\n```\nstruct mu {\n sync.Mutex\n fieldsProtectedByMutex...\n}\n```\n\n---\n\n*[server/status/recorder.go, line 125 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/4584#-KBAZwyD4P6kBw7H9Qfm:-KBAZwyD4P6kBw7H9Qfn:140394890) ([raw file](https://github.com/cockroachdb/cockroach/blob/316ef65170f9b40b131616634b127abd1cd569a7/server/status/recorder.go#L125)):*\nremove this comma\n\n---\n\n*[server/status/recorder.go, line 154 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/4584#-KBA_7Rof7Lp-2aLdz6F:-KBA_7Rof7Lp-2aLdz6G:-714432608) ([raw file](https://github.com/cockroachdb/cockroach/blob/316ef65170f9b40b131616634b127abd1cd569a7/server/status/recorder.go#L154)):*\nthat's not an empty byte string, it's an empty json object.\n\n---\n\n*[server/status/recorder.go, line 193 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/4584#-KBA_QkV6_i0TSHJ8g2-:-KBA_QkV6_i0TSHJ8g20:-1393024112) ([raw file](https://github.com/cockroachdb/cockroach/blob/316ef65170f9b40b131616634b127abd1cd569a7/server/status/recorder.go#L193)):*\nnit: strconv.Itoa avoids the cast\n\n---\n\n*[server/status/recorder.go, line 309 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/4584#-KBA_qI7Jl85ZzdRYzIM:-KBA_qI7Jl85ZzdRYzIN:-234941509) ([raw file](https://github.com/cockroachdb/cockroach/blob/316ef65170f9b40b131616634b127abd1cd569a7/server/status/recorder.go#L309)):*\ncan you put a comment here? i can't tell that `prefix` is a format string\n\n---\n\n*[storage/store.go, line 385 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/4584#-KBAaAYdWEHOMzPwUE_E:-KBAaAYdWEHOMzPwUE_F:-19325394) ([raw file](https://github.com/cockroachdb/cockroach/blob/316ef65170f9b40b131616634b127abd1cd569a7/storage/store.go#L385)):*\ns/below/above/?\n\n---\n\n*[util/metric/metric.go, line 247 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/4584#-KBAUkUn-OJUuJeeZfPf:-KBAUkUoLuwJUoGsBPcP:677713193) ([raw file](https://github.com/cockroachdb/cockroach/blob/316ef65170f9b40b131616634b127abd1cd569a7/util/metric/metric.go#L247)):*\nhuh?\n\n---\n\n*[util/metric/registry_test.go, line 71 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/4584#-KBAV8RQAEel6T8lr6F9:-KBAV8RQAEel6T8lr6FA:1636531087) ([raw file](https://github.com/cockroachdb/cockroach/blob/316ef65170f9b40b131616634b127abd1cd569a7/util/metric/registry_test.go#L71)):*\nthe cases that expect nil and get non-nil should probably include the non-nil value in the error message. the case that expect non-nil should probably have a slightly stronger assertion that the right value is returned (is that possible?).\n\n---\n\n\n*Comments from the [review on Reviewable.io](https://reviewable.io:443/reviews/cockroachdb/cockroach/4584#-:-KBAaKtCY7blpaRUfwgG:-786036987)*\n<!-- Sent from Reviewable.io -->\n"}}