Circonus metrics adapter by redhotpenguin · Pull Request #1737 · istio/istio

redhotpenguin · 2017-11-15T23:09:33Z

What this PR does / why we need it:

This PR adds a metrics adapter for Circonus, similar to the Prometheus and Statsd adapters. It draws from my previous PR for the 0.1 based metrics implementation - this has been updated to use the new handler structure. It uses the ScheduleWork() call per the adapter development guide to schedule flushes to Circonus.

The tests are fashioned after the existing adapter tests. I implemented a version using testenv, but I wasn't able to access the adapter. It looks like it is possible, but will require a bit more understanding on my part, and possible additional test methods.

I've verified that the adapter works as advertised both in mixc client environment as well as with the BookInfo sample in GKE. I included a sample operator config - I didn't see that with the additional adapters, but think it will be useful to the end user.

Please let me know how this looks, any suggestions are welcome.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Unit tests don't need an http trap submission url, a fake one is part of the testdata config.

Release note:

This change introduces a metrics adapter for Circonus metrics collection.

istio-testing · 2017-11-15T23:09:40Z

Hi @redhotpenguin. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

ZackButcher · 2017-11-15T23:38:10Z

/ok-to-test

redhotpenguin · 2017-11-17T00:43:48Z

/retest

redhotpenguin · 2017-11-17T01:12:16Z

Dropping a ping to @geeknoid for a review as suggested reviewer. Looks like the circleci tests that failed are an envoy issue - not sure if related.

geeknoid

Thanks for the adapter.

geeknoid · 2017-11-17T14:26:18Z

mixer/adapter/circonus/circonus.go

Extra line?

geeknoid · 2017-11-17T14:26:51Z

mixer/adapter/circonus/circonus.go

Initialize with struct init syntax instead?

Done, and additional fmt/linter/doc changes have been pushed.

geeknoid · 2017-11-17T14:28:28Z

mixer/adapter/circonus/circonus.go

Please run "go fmt" on the file.

Our linter bot is currently broken, so you won't get dinged during the presubmit tests unfortunately. We're working on fixing that.

Huh, surprised Goland didn't gofmt automatically, I thought it had been. Good to know.

Thanks for the feedback; I'll make these requested changes and run the tests again.

geeknoid · 2017-11-17T14:31:48Z

mixer/adapter/circonus/circonus.go

I think you need to have some logic here to validate your config. is submission_url a well-formed URL? Are metric names supplied, and other fields have valid values, etc.

Probably should give an error if you've been giving a particular MetricType via SetMetricTypes, but your adapter config doesn't specify that particular metric.

geeknoid · 2017-11-17T14:33:36Z

mixer/adapter/circonus/circonus.go

Adapters don't need to check for this case as it would indicate a bug in the Mixer. So we just assume this works and expect a crash later on due to nil pointers if that's not the case.

geeknoid · 2017-11-17T14:35:04Z

mixer/adapter/circonus/circonus.go

Redundant map lookup, already did it right above.

geeknoid · 2017-11-17T14:41:50Z

mixer/adapter/circonus/circonus.go

This should be caught at config validation time (assuming you update the Validate method), so at runtime you can just assume it worked.

Same comment applies to other validations here...

geeknoid · 2017-11-17T14:44:58Z

mixer/adapter/circonus/circonus.go

It would be better if the interaction with the backend were handled synchronously so that errors are reported up. As it is, errors in this flush code will be invisible to the rest of the Istio pipeline.

I was able to tie the circonus-gometrics logger which uses the standard log package into the env.Logger() interface. Log entries are being reported with the async approach for flush events. Let me know if this handles the spirit of what you were after here.

geeknoid · 2017-11-17T18:18:39Z

Yeah, GoLand no longer seems to be running go fmt for me since the last update. I've got it setup as a File Watching thingy, but it's not triggering.

…

On Fri, Nov 17, 2017 at 9:19 AM, Fred Moyer ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In mixer/adapter/circonus/circonus.go <#1737 (comment)>: > + cmc.CheckManager.Check.SubmissionURL = b.adpCfg.SubmissionUrl + cmc.Debug = true + cmc.Interval = "0" + cm, err := cgm.NewCirconusMetrics(cmc) + if err != nil { + err = env.Logger().Errorf("Could not create NewCirconusMetrics: %v", err) + return nil, err + } + + metrics := make(map[string]config.Params_MetricInfo_Type) + ac := b.adpCfg + for _, metric := range ac.Metrics { + + metrics[metric.Name] = metric.Type + } + return &handler{cm: *cm, metricTypes: b.metricTypes, env: env, metrics: metrics}, nil Huh, surprised Goland didn't gofmt automatically, I thought it had been. Good to know. Thanks for the feedback; I'll make these requested changes and run the tests again. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1737 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AVucHYN5mfufRXdq4sgowKgbOK4U4kR3ks5s3cAhgaJpZM4Qftx8> .

ijsnellf · 2017-11-20T21:29:44Z

mixer/adapter/circonus/circonus.go

use standard godoc style here?

// Build does x and y

Noted - will make this change. Took this from the adapter developer's guide example.

ijsnellf · 2017-11-20T21:33:11Z

mixer/adapter/circonus/circonus.go

style nit: we typically don't have large comment headers in our code

codecov · 2017-11-21T17:17:06Z

Codecov Report

Merging #1737 into master will decrease coverage by 0.24%.
The diff coverage is 69.23%.

@@            Coverage Diff             @@
##           master    #1737      +/-   ##
==========================================
- Coverage   82.08%   81.84%   -0.25%     
==========================================
  Files         198      192       -6     
  Lines       16320    15919     -401     
==========================================
- Hits        13397    13029     -368     
+ Misses       2480     2428      -52     
- Partials      443      462      +19

Flag	Coverage Δ
#broker	`47.27% <ø> (ø)`	⬆️
#mixer	`83.35% <69.23%> (-0.35%)`	⬇️
#pilot	`80.78% <ø> (+0.2%)`	⬆️
#security	`92.03% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
mixer/adapter/circonus/circonus.go	`69.23% <69.23%> (ø)`
mixer/adapter/prometheus/server.go	`89.58% <0%> (-6.25%)`	⬇️
mixer/adapter/denier/denier.go	`88.23% <0%> (-3.87%)`	⬇️
mixer/adapter/stackdriver/metric/bufferedClient.go	`90.32% <0%> (-3.23%)`	⬇️
mixer/adapter/list/stringList.go	`100% <0%> (ø)`	⬆️
mixer/pkg/aspect/descriptors.go	`100% <0%> (ø)`	⬆️
mixer/adapter/memquota/rollingWindow.go	`100% <0%> (ø)`	⬆️
mixer/pkg/runtime/init.go
mixer/pkg/runtime/resourceType.go
mixer/pkg/runtime/dispatcher.go
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6889c52...22e1ce3. Read the comment docs.

redhotpenguin · 2017-11-27T18:25:56Z

@geeknoid @ijsnellf let me know if you can take a look at the updates I pushed last week. Resolved all fmt/linting issues, and bridged to the env logger so any scheduled events will report in the log (in addition to synchronous events). Thanks.

geeknoid · 2017-11-27T19:09:46Z

Sorry, I didn't realize you were ready for another review. Our custom is to add a comment with PTAL in it so reviewers get a heads-up. I'll look shortly. Thanks.

…

On Mon, Nov 27, 2017 at 10:26 AM, Fred Moyer ***@***.***> wrote: @geeknoid <https://github.com/geeknoid> @ijsnellf <https://github.com/ijsnellf> let me know if you can take a look at the updates I pushed last week. Resolved all fmt/linting issues, and bridged to the env logger so any scheduled events will report in the log (in addition to synchronous events). Thanks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1737 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AVucHd5FCG14c77HLrJFoaJb2_WvZGenks5s6v68gaJpZM4Qftx8> .

geeknoid · 2017-11-27T19:29:38Z

mixer/adapter/circonus/circonus.go

Should the interval be in config?

I'm manually flushing here, so this needs to be set to 0.

geeknoid · 2017-11-27T19:31:00Z

mixer/adapter/circonus/circonus.go

Would it make sense to complain about entries defined in your config, but for which there is no MetricType?

geeknoid · 2017-11-27T19:35:00Z

mixer/adapter/circonus/circonus.go

You probably should synchronously flush here, otherwise your buffered data would never make it out.

Thanks for the spot, will make that update.

I just realized you're flushing for every HandleMetric call, so there is no buffered state to flush in Close...

geeknoid · 2017-11-27T19:42:54Z

mixer/adapter/circonus/circonus.go

Is this just for debugging? This could be called at very high QPS, so flushing on every call seems unfortunate.

It was intentional; I had the same concern also, but wanted to follow this approach as opposed to letting cgm autoflush and not have the async work wrapped by ScheduleWork as documented in the developer guide. Let me dig into the flush call again and see if there's a way to pass the autoflush call to ScheduleWork.

The reason we use ScheduleWork is so that we can trap and recover from panics in the code, and not crash/deadlock Mixer as a whole for a local adapter failure. This is definitely not very composable with existing packages that spawn their own goroutines.

If you can't wrap their autoflush, then you should probably use env.ScheduleDaemon and start your own auto-flusher goroutine that calls the flush call on a configurable period.

Ok I'll work on implementing that and push. Thanks for the feedback here; helps a lot.

istio-merge-robot · 2017-11-29T00:22:55Z

@redhotpenguin PR needs rebase

rshriram · 2017-11-29T05:04:47Z

can you also sync with master, so that you run the right set of circleci unit tests?

* Bridge cgm log to mixer glog implementation to set cgm logger * Cgm constructor using struct declaration, not field assignment * Rework function documentation * Add configuration validation * Remove testenv dep and update istio api dep * Linter fixes

* Initialize startup background tasks under ScheduleWork() * Flush on an interval under ScheduleWork() * Additional config and metrictype validation

* Initialize struct in block syntax * Remove Close() test which did not test anything meaningful

* Use a context WithCancel approach to shut down the flush goroutine * Add validation for FlushInterval less than 1 second * Add unit test for validation error cases

rshriram · 2017-12-05T00:04:38Z

and the funky robot commands don't work with circleci :). They are meant only for prow..

redhotpenguin · 2017-12-05T00:05:01Z

@rshriram thanks - still learning the ropes here with the bot commands :)

redhotpenguin · 2017-12-05T00:37:08Z

@rshriram all tests pass, thanks for the help getting through that. Looks like the only thing remaining is the lgtm label.

geeknoid · 2017-12-05T03:08:28Z

/lgtm
/approve

istio-merge-robot · 2017-12-05T03:08:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: geeknoid

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [geeknoid]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

istio-merge-robot · 2017-12-05T03:08:46Z

/test all [submit-queue is verifying that this PR is safe to merge]

istio-merge-robot · 2017-12-05T04:28:58Z

/test all [submit-queue is verifying that this PR is safe to merge]

istio-merge-robot · 2017-12-05T04:55:39Z

Automatic merge from submit-queue.

istio-testing added the release-note label Nov 15, 2017

googlebot added the cla: yes label Nov 15, 2017

istio-testing added the needs-ok-to-test label Nov 15, 2017

istio-testing removed the needs-ok-to-test label Nov 15, 2017

geeknoid reviewed Nov 17, 2017

View reviewed changes

yutongz added do-not-merge/post-submit and removed do-not-merge/post-submit labels Nov 17, 2017

ijsnellf reviewed Nov 20, 2017

View reviewed changes

yutongz added do-not-merge/post-submit and removed do-not-merge/post-submit labels Nov 22, 2017

geeknoid reviewed Nov 27, 2017

View reviewed changes

yutongz added the do-not-merge/post-submit label Nov 28, 2017

istio-merge-robot added do-not-merge/post-submit and removed do-not-merge/post-submit labels Nov 28, 2017

istio-merge-robot added needs-rebase Indicates a PR needs to be rebased before being merged do-not-merge/post-submit and removed do-not-merge/post-submit labels Nov 29, 2017

redhotpenguin added 10 commits December 4, 2017 16:03

Use the Logger interface rather than calling glog

81548dd

Flush scheduling and validation changes

9d57898

* Initialize startup background tasks under ScheduleWork() * Flush on an interval under ScheduleWork() * Additional config and metrictype validation

Remove test that failed needlessly.

4538328

* Initialize struct in block syntax * Remove Close() test which did not test anything meaningful

Goland 2017.3 reformat misses imports, remove type

a7abbde

Format with fmt.sh

7e5fe34

Pass cgm by ref instead of value, grammar fix

3baacc8

Properly clean up flush goroutine, add validation

18efc42

* Use a context WithCancel approach to shut down the flush goroutine * Add validation for FlushInterval less than 1 second * Add unit test for validation error cases

Add circonus-gometrics to resolve circle-ci issue

7453532

Revert adding cgm to Gopkg.toml for cache issue

cfe9387

istio-merge-robot added the do-not-merge/post-submit label Dec 5, 2017

istio-merge-robot added do-not-merge/post-submit and removed do-not-merge/post-submit labels Dec 5, 2017

istio-testing assigned geeknoid Dec 5, 2017

istio-testing added the lgtm label Dec 5, 2017

istio-merge-robot added do-not-merge/post-submit PostSubmit Failed/Contact Oncall and removed do-not-merge/post-submit PostSubmit Failed/Contact Oncall labels Dec 5, 2017

istio-merge-robot merged commit 1362e71 into istio:master Dec 5, 2017

Conversation

redhotpenguin commented Nov 15, 2017

Uh oh!

istio-testing commented Nov 15, 2017

Uh oh!

ZackButcher commented Nov 15, 2017

Uh oh!

redhotpenguin commented Nov 17, 2017

Uh oh!

redhotpenguin commented Nov 17, 2017

Uh oh!

geeknoid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geeknoid commented Nov 17, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

redhotpenguin commented Nov 27, 2017

Uh oh!

geeknoid commented Nov 27, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

istio-merge-robot commented Nov 29, 2017

Uh oh!

rshriram commented Nov 29, 2017

Uh oh!

codecov bot commented Nov 21, 2017 •

edited

Loading