Convert Mixer self-monitoring to use OpenCensus libraries #7989

douglas-reid · 2018-08-16T17:44:13Z

This PR is part of an exploration of moving Istio self-monitoring from pure Prometheus libraries to OpenCensus (OC) libraries. The ultimate goal being explored is the possibility of creating an Istio OpenCensus exporter mechanism that would allow flexible export of Istio self-monitoring metrics to different backends (cloudwatch, datadog, stackdriver, etc.).

This PR does the following:

adds an OC gRPC stats handler to replace the Prometheus gRPC interceptor
tries to track down and remove all self-monitoring bits using Prometheus libraries (checkcache, runtime)
configures a Prometheus exporter for now (on same self-monitoring port)

This PR does not replace the Prometheus collectors that provide additional metrics (not from Mixer). These include:

Golang collector (for gauges on go-routines, etc.)
Process collector (linux process monitoring bits)

Reviewers:
I am interested in feedback on a few items. First, does this library change make sense? Second, is the way that OC is being used seem appropriate?

Important items to note when reviewing:
OC models things differently to prometheus and this results in some interesting changes:

For gauge aggregations, there isn't any way currently to express "last value +/- delta". So, in places where the codebase was using that sort of logic, new logic was added to separately track the value.
In establishing views, I've chosen here to mostly use view.Count. This is primarily because that results in exporting Prometheus metrics of type Counter. Using view.Sum results in untyped prometheus metrics. This ultimately impacts how we use the Stats, as view.Count records the number of measurements, not the total of those measurements. I think this is the right thing to do for now, but maybe not. The Prometheus docs include the following related documentation:

The Prometheus server does not yet make use of the type information and flattens all data into untyped time series. This may change in the future.

Because OC does not export stats for which no measurements that have been recorded, the overall output to prometheus will be different than before (we won't see 0s for things that haven't happened, etc.)

codecov · 2018-08-16T17:55:09Z

Codecov Report

Merging #7989 into master will increase coverage by 1%.
The diff coverage is 91%.

@@          Coverage Diff           @@
##           master   #7989   +/-   ##
======================================
+ Coverage      71%     71%   +1%     
======================================
  Files         374     370    -4     
  Lines       32673   32620   -53     
======================================
+ Hits        22906   22937   +31     
+ Misses       8760    8692   -68     
+ Partials     1007     991   -16

Impacted Files	Coverage Δ
mixer/pkg/runtime/routing/table.go	`100% <ø> (ø)`	⬆️
mixer/pkg/server/server.go	`97% <100%> (+7%)`	⬆️
mixer/pkg/runtime/dispatcher/dispatchstate.go	`94% <100%> (+1%)`	⬆️
mixer/pkg/checkcache/cache.go	`100% <100%> (ø)`	⬆️
mixer/pkg/runtime/dispatcher/session.go	`93% <50%> (-1%)`	⬇️
mixer/pkg/runtime/config/snapshot.go	`22% <72%> (+4%)`	⬆️
mixer/pkg/runtime/routing/builder.go	`68% <72%> (ø)`	⬇️
mixer/pkg/server/monitoring.go	`96% <75%> (+1%)`	⬆️
mixer/pkg/runtime/handler/table.go	`77% <82%> (-1%)`	⬇️
mixer/pkg/runtime/handler/env.go	`96% <90%> (+11%)`	⬆️
... and 52 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 5a7ce28...c76dd92. Read the comment docs.

douglas-reid · 2018-08-16T18:03:13Z

cc: @Ramonza

geeknoid

This looks pretty good.

Is there a solution to deal with the metrics used by our dependencies? This is an identical problem we had when replacing glog, which led to the istio/glog repo. Do we need something similar here?

geeknoid · 2018-08-16T18:50:29Z

mixer/pkg/checkcache/cache.go

 	return nil
 }

 // Get looks up an attribute bag in the cache.
 func (cc *Cache) Get(attrs attribute.Bag) (Value, bool) {
+	defer func() { cc.recordStats() }()


Here and elsewhere in this file, I'm not thrilled with "defer" here since it induces an allocation (last I looked)

geeknoid · 2018-08-16T18:55:26Z

mixer/pkg/server/server.go

+
+	exporter, err := prometheus.NewExporter(prometheus.Options{Registry: oprometheus.DefaultRegisterer.(*oprometheus.Registry)})
+	if err != nil {
+		return nil, fmt.Errorf("could not build prometheus exporter: %v", err)


Need to add a unit test to cover this new code.

geeknoid · 2018-08-16T18:55:39Z

mixer/pkg/server/server.go

+
+	// Register the views to collect server request count.
+	if err := view.Register(ocgrpc.DefaultServerViews...); err != nil {
+		return nil, fmt.Errorf("could not register default server views: %v", err)


Need unit test for this.

semistrict · 2018-08-16T18:21:05Z

mixer/pkg/checkcache/cache.go

+	missesTotal    = stats.Int64("mixer/checkcache/cache_misses_total", "The number of times a cache lookup operation failed to find an entry in the cache.", stats.UnitDimensionless)
+	evictionsTotal = stats.Int64("mixer/checkcache/cache_evictions_total", "The number of entries that have been evicted from the cache.", stats.UnitDimensionless)
+
+	writesView    = newView(writesTotal, []tag.Key{}, view.Sum())


I think you want the LastValue aggregation type here. Otherwise, with Sum you will keep adding to a cumulative total and it looks like the actual values you're recording already represent a cumulative total so you really just want to record the value and not total up all the values you ever passed to stats.Record I think.

Great catch!

semistrict · 2018-08-16T18:21:21Z

mixer/pkg/checkcache/cache.go

+
+func newView(measure stats.Measure, keys []tag.Key, aggregation *view.Aggregation) *view.View {
+	return &view.View{
+		Name:        measure.Name(),


FWIW, this is the default if no name is specified

semistrict · 2018-08-16T18:23:46Z

mixer/pkg/checkcache/cache.go

@@ -200,3 +183,10 @@ func (cc *Cache) Set(attrs attribute.Bag, value Value) {

 	cc.cache.SetWithExpiration(shape.makeKey(attrs), value, value.Expiration.Sub(now))
 }
+
+func (cc *Cache) recordStats() {
+	stats.Record(context.Background(), writesTotal.M(int64(cc.cache.Stats().Writes)))


This can be coalesced into a single Record call:

cs := cc.cache.Stats() stats.Record( context.Background(), hitsTotal.M(int64(cs.Hits)), missesTotal.M(int64(cs.Misses)), ...

semistrict · 2018-08-16T19:22:04Z

mixer/pkg/runtime/dispatcher/session.go

@@ -105,7 +107,9 @@ func (s *session) dispatch() error {
 	namespace, err := getIdentityNamespace(s.bag)
 	if err != nil {
 		// early return.
-		updateRequestCounters(0, 0)
+		stats.Record(s.ctx, monitoring.DestinationsPerRequest.M(0))


Can be coalesced to a single Record call

semistrict · 2018-08-16T19:25:58Z

mixer/pkg/runtime/dispatcher/session.go

@@ -186,7 +190,9 @@ func (s *session) dispatch() error {
 		}
 	}

-	updateRequestCounters(ndestinations, ninputs)
+	stats.Record(s.ctx, monitoring.DestinationsPerRequest.M(int64(ndestinations)))


douglas-reid · 2018-08-22T00:01:32Z

Reviewers: PTAL.

I believe I have addressed the existing comments (adding unit tests, etc.). I have also updated the e2e_dashboard test (passes on local clusters) and the Mixer dashboard that is exercised by that test.

douglas-reid · 2018-08-22T00:07:11Z

@geeknoid was there something you had in mind re: metrics used by our deps? I was not aware of any dependencies that were using prometheus to monitor anything prior to this PR.

I don't know that we need to solve that particular issue right now, but it does raise an interesting question of package instrumentation and playing well with others. @Ramonza does OC have any thoughts on such issues?

douglas-reid · 2018-08-22T00:41:00Z

/test istio-unit-tests

and, of course, linting produces different results on every run... sigh.

geeknoid · 2018-08-22T12:00:16Z

Sorry for my unclear comment. I was referring to this from your PR comment:

This PR does not replace the Prometheus collectors that provide additional metrics (not from Mixer). These include:

Golang collector (for gauges on go-routines, etc.)
Process collector (linux process monitoring bits)

Can these be addressed too at some point in a systematic way?

On a related note, the ControlZ library is currently mining Prometheus metrics for display in the UI. What happens in ControlZ after this PR goes in?

douglas-reid · 2018-08-22T15:41:05Z

@geeknoid ah, gotcha. Yes, probably. But rather than duplicate that functionality in this PR (the key is really finding some hook to generate them on the fly the same way prometheus does, I believe), I've skipped over providing those in an OpenCensus way. I think, ultimately, it probably makes more sense for OpenCensus to provide those measures than for Istio to build them. For now, skipping them seems fine. @Ramonza thoughts here?

RE ControlZ:

It still works. We use the default registry (gatherer) for the prometheus exporter here, and that is what is currently powering the metrics portion of ControlZ.

geeknoid · 2018-08-22T16:39:39Z

/approve /lgtm

…

On Wed, Aug 22, 2018 at 8:42 AM Douglas Reid ***@***.***> wrote: @geeknoid <https://github.com/geeknoid> ah, gotcha. Yes, probably. But rather than duplicate that functionality in this PR (the key is really finding some hook to generate them on the fly the same way prometheus does, I believe), I've skipped over providing those in an OpenCensus way. I think, ultimately, it probably makes more sense for OpenCensus to provide those measures than for Istio to build them. For now, skipping them seems fine. @Ramonza <https://github.com/ramonza> thoughts here? RE ControlZ: It still works. We use the default registry (gatherer) for the prometheus exporter here, and that is what is currently powering the metrics portion of ControlZ. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7989 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AVucHYZ8x4mX7qP9bQWH-hf9VylOus0yks5uTXvvgaJpZM4WAS-9> .

douglas-reid · 2018-08-23T05:15:18Z

Thanks for all the reviews! I've fixed up all of the linter issues and dep diff stuff now. PTAL for final approval.

geeknoid · 2018-08-23T15:35:28Z

/lgtm
/approve

istio-testing · 2018-08-23T15:35:36Z

[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.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [geeknoid]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

istio-testing · 2018-08-23T20:01:26Z

@douglas-reid: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/istio-pilot-multicluster-e2e.sh	`c76dd92`	link	`/test istio-pilot-multicluster-e2e`

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.

googlebot added the cla: yes label Aug 16, 2018

douglas-reid requested review from geeknoid and bianpengyuan August 16, 2018 17:55

douglas-reid added the do-not-merge/hold Block automatic merging of a PR. label Aug 16, 2018

geeknoid reviewed Aug 16, 2018

View reviewed changes

semistrict reviewed Aug 16, 2018

View reviewed changes

douglas-reid force-pushed the mixer-opencensus branch from ffbed4c to 950fb11 Compare August 21, 2018 16:28

douglas-reid changed the title ~~WIP: Convert Mixer self-monitoring to use OpenCensus libraries~~ Convert Mixer self-monitoring to use OpenCensus libraries Aug 22, 2018

douglas-reid removed the do-not-merge/hold Block automatic merging of a PR. label Aug 22, 2018

istio-testing assigned geeknoid Aug 22, 2018

istio-testing added lgtm and removed lgtm labels Aug 22, 2018

douglas-reid added 9 commits August 22, 2018 17:28

WIP: OpenCensus conversion for Mixer

29e1d86

WIP: Part 2

505e8f4

Remove prometheus grpc interceptor

0872159

Unit tests for server

23e81ff

Improve test coverage

b3abc24

Consolidate stats.Record calls

f7e21de

Remove defers

5732c4c

Sum -> LastValue for cache aggregations

d63a834

Fix typo in unit test

1cd7a63

douglas-reid and others added 6 commits August 22, 2018 17:29

Linter error fix

b4b895d

Update dashboard to match metrics update

e288150

Linter fixes (context first, lll patches, etc.)

8790c88

Please linters, let me sleep tonight

4f5a2bd

stats improvement

1fda1c0

touching the lock file and a vendor file because reasons

c76dd92

douglas-reid force-pushed the mixer-opencensus branch from c01f2fb to c76dd92 Compare August 23, 2018 00:37

istio-testing added the lgtm label Aug 23, 2018

istio-testing merged commit 6faa6cb into istio:master Aug 23, 2018

clyang82 mentioned this pull request Nov 19, 2018

citadel uses OpenCensus for self-monitoring #10048

Merged

douglas-reid deleted the mixer-opencensus branch August 27, 2019 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert Mixer self-monitoring to use OpenCensus libraries #7989

Convert Mixer self-monitoring to use OpenCensus libraries #7989

douglas-reid commented Aug 16, 2018 •

edited

codecov bot commented Aug 16, 2018 •

edited

douglas-reid commented Aug 16, 2018

geeknoid left a comment

geeknoid Aug 16, 2018

douglas-reid Aug 21, 2018

geeknoid Aug 16, 2018

geeknoid Aug 16, 2018

semistrict Aug 16, 2018

douglas-reid Aug 21, 2018

semistrict Aug 16, 2018

semistrict Aug 16, 2018

semistrict Aug 16, 2018

semistrict Aug 16, 2018

douglas-reid commented Aug 22, 2018

douglas-reid commented Aug 22, 2018

douglas-reid commented Aug 22, 2018

geeknoid commented Aug 22, 2018

douglas-reid commented Aug 22, 2018

geeknoid commented Aug 22, 2018 via email

douglas-reid commented Aug 23, 2018

geeknoid commented Aug 23, 2018

istio-testing commented Aug 23, 2018

istio-testing commented Aug 23, 2018

Convert Mixer self-monitoring to use OpenCensus libraries #7989

Convert Mixer self-monitoring to use OpenCensus libraries #7989

Conversation

douglas-reid commented Aug 16, 2018 • edited

codecov bot commented Aug 16, 2018 • edited

Codecov Report

douglas-reid commented Aug 16, 2018

geeknoid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

douglas-reid commented Aug 22, 2018

douglas-reid commented Aug 22, 2018

douglas-reid commented Aug 22, 2018

geeknoid commented Aug 22, 2018

douglas-reid commented Aug 22, 2018

geeknoid commented Aug 22, 2018 via email

douglas-reid commented Aug 23, 2018

geeknoid commented Aug 23, 2018

istio-testing commented Aug 23, 2018

istio-testing commented Aug 23, 2018

douglas-reid commented Aug 16, 2018 •

edited

codecov bot commented Aug 16, 2018 •

edited