{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/25227","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/25227/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/25227/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/25227/events","html_url":"https://github.com/cockroachdb/cockroach/pull/25227","id":319375104,"number":25227,"title":"[util/metric] Add GraphiteExporter","user":{"login":"neeral","id":8496732,"avatar_url":"https://avatars0.githubusercontent.com/u/8496732?v=4","gravatar_id":"","url":"https://api.github.com/users/neeral","html_url":"https://github.com/neeral","followers_url":"https://api.github.com/users/neeral/followers","following_url":"https://api.github.com/users/neeral/following{/other_user}","gists_url":"https://api.github.com/users/neeral/gists{/gist_id}","starred_url":"https://api.github.com/users/neeral/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/neeral/subscriptions","organizations_url":"https://api.github.com/users/neeral/orgs","repos_url":"https://api.github.com/users/neeral/repos","events_url":"https://api.github.com/users/neeral/events{/privacy}","received_events_url":"https://api.github.com/users/neeral/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":3,"created_at":"2018-05-02T00:46:37Z","updated_at":"2018-05-05T04:39:07Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/25227","html_url":"https://github.com/cockroachdb/cockroach/pull/25227","diff_url":"https://github.com/cockroachdb/cockroach/pull/25227.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/25227.patch"},"body":"See issue #25198 for more context on the motivation. For clusters which use the Graphite stack instead of Prometheus for monitoring, we need a way to export metrics. Prometheus has a library that performs the conversion.\r\n\r\nTo do / discuss:\r\n- update dependencies in vendor directory\r\n- how to feature toggle this on/off\r\n- how to pass in config values (URL for graphite server, polling interval), e.g. cmd-line args\r\n- there might be a couple of other questions I've asked in my code\r\n\r\nThis PR isn't polished as I want some feedback before choosing e.g. to use cmd-line args for config."},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/386778646","html_url":"https://github.com/cockroachdb/cockroach/pull/25227#issuecomment-386778646","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/25227","id":386778646,"user":{"login":"tschottdorf","id":5076964,"avatar_url":"https://avatars1.githubusercontent.com/u/5076964?v=4","gravatar_id":"","url":"https://api.github.com/users/tschottdorf","html_url":"https://github.com/tschottdorf","followers_url":"https://api.github.com/users/tschottdorf/followers","following_url":"https://api.github.com/users/tschottdorf/following{/other_user}","gists_url":"https://api.github.com/users/tschottdorf/gists{/gist_id}","starred_url":"https://api.github.com/users/tschottdorf/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tschottdorf/subscriptions","organizations_url":"https://api.github.com/users/tschottdorf/orgs","repos_url":"https://api.github.com/users/tschottdorf/repos","events_url":"https://api.github.com/users/tschottdorf/events{/privacy}","received_events_url":"https://api.github.com/users/tschottdorf/received_events","type":"User","site_admin":false},"created_at":"2018-05-05T04:39:06Z","updated_at":"2018-05-05T04:39:06Z","author_association":"OWNER","body":"I agree with @mrtracy that a cluster setting is the way to go here. Strawman name: `external.graphite_server`. You would set this to something like `graphite://localhost:9002` (http? https?)\n\nAs per my comments below, it seems that you can use the ticker loop to create a new bridge from the current URL every time (doing nothing if there isn't one). Is that accurate or am I overlooking something?\n\n---\n\nReviewed 3 of 3 files at r1.\nReview status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.\n\n---\n\n*[pkg/server/server.go, line 1426 at r1](https://reviewable.io/reviews/cockroachdb/cockroach/25227#-LBTJBsZqAIsBDlmtXKj-r1-1426:-LBiZiio7b3BORqdEOH1:bdjc6lh) ([raw file](https://github.com/cockroachdb/cockroach/blob/851e1a1d5ddd5ac2eae479000b4a97253d843347/pkg/server/server.go#L1426)):*\n<details><summary><i>Previously, mrtracy (Matt Tracy) wrote…</i></summary><blockquote>\n\nIf our configuration is based on a cluster setting, that implies that the push process can start (or stop) after the cockroach process.  That means you will need to change where your RunWorker loop is initialized - or perhaps as an alternative, that task will need to be \"pauseable\".\n</blockquote></details>\n\nPer my comment below, I think we can leave this loop here but init the graphite settings every time the ticker fires. That way, it would be a no-op until the cluster settings come in.\n\n---\n\n*[pkg/util/metric/graphite_exporter.go, line 26 at r1](https://reviewable.io/reviews/cockroachdb/cockroach/25227#-LBTJBsZqAIsBDlmtXKl-r1-26:-LBiZGhq6AlzxE47apz9:by61xrv) ([raw file](https://github.com/cockroachdb/cockroach/blob/851e1a1d5ddd5ac2eae479000b4a97253d843347/pkg/util/metric/graphite_exporter.go#L26)):*\n<details><summary><i>Previously, mrtracy (Matt Tracy) wrote…</i></summary><blockquote>\n\nSince this constructs the graphite \"Bridge\", which has the configured URL and Timeout and such, the GraphiteExporter will also need to respond to cluster settings; you won't be able to depend on setting these variables once at start time.\n</blockquote></details>\n\nI think the simplest way to do this is to have a periodic loop that creates a one-shot Graphite exporter from the current cluster settings every time it runs. There's no need to have a persistent object here, right? This just posts somewhere and then that's that.\n\n---\n\n*[pkg/util/metric/graphite_exporter.go, line 55 at r1](https://reviewable.io/reviews/cockroachdb/cockroach/25227#-LBi_uBy2FRhecaTWGSq:-LBi_uBy2FRhecaTWGSr:buzc58k) ([raw file](https://github.com/cockroachdb/cockroach/blob/851e1a1d5ddd5ac2eae479000b4a97253d843347/pkg/util/metric/graphite_exporter.go#L55)):*\n> ```Go\n> \n> func (ge *GraphiteExporter) Gather() ([]*prometheusgo.MetricFamily, error) {\n> \tnum_metrics := 0\n> ```\n\nunused?\n\n---\n\n*[pkg/util/metric/graphite_exporter.go, line 63 at r1](https://reviewable.io/reviews/cockroachdb/cockroach/25227#-LBi_vNN32akz3Iq-OF5:-LBi_vNN32akz3Iq-OF6:buzc58k) ([raw file](https://github.com/cockroachdb/cockroach/blob/851e1a1d5ddd5ac2eae479000b4a97253d843347/pkg/util/metric/graphite_exporter.go#L63)):*\n> ```Go\n> \tfor _, family := range ge.pm.families {\n> \t\tv[i] = family\n> \t\ti++\n> ```\n\nunused?\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io/reviews/cockroachdb/cockroach/25227#-:-LBiZw9N-AJal5WtRqSB:bmo5oau)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"tschottdorf"}}