Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Allow creating additional View universes. #1196

Merged
merged 7 commits into from
Feb 18, 2020

Conversation

evankanderson
Copy link
Contributor

Fixes #1195

I wanted to be able to export statistics for multiple Resources, but I discovered that all the stats aggregation was bundled into a single "universe" in the stats/view defaultWorker. This PR moves all the singleton state into a Worker interface, and maintains the simple singleton interface for the common case, but allows creating additional Workers in complex cases.

@evankanderson
Copy link
Contributor Author

CC @anniefu

@evankanderson
Copy link
Contributor Author

I added an additional interface for extracting the stats.Option settings for passing to the view.Record() method. This seemed the simplest way to extract the logic in stats.RecordWithOptions, which currently consumes the context and performs some short-circuiting logic.

//
// Note that this is an advanced use case, and the static functions in this
// module should cover the common use cases.
type Worker interface {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

open-telemetry has concept of Meter, where you can create multiple meters each having their exporter. The worker is similar to that concept. I would probably rename Worker to Meter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, happy to do so.

stats/view/worker.go Show resolved Hide resolved
// Note that this is an advanced use case, and the static functions in this
// module should cover the common use cases.
type Worker interface {
Record(tags *tag.Map, ms []stats.Measurement, attachments map[string]interface{})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add doc for each method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

stats/view/worker.go Show resolved Hide resolved
stats/record.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated and responded to comments, thanks!

stats/view/worker.go Show resolved Hide resolved
// Note that this is an advanced use case, and the static functions in this
// module should cover the common use cases.
type Worker interface {
Record(tags *tag.Map, ms []stats.Measurement, attachments map[string]interface{})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

stats/record.go Outdated Show resolved Hide resolved
@rghetia rghetia requested a review from songy23 February 8, 2020 00:15
@evankanderson evankanderson force-pushed the multi-metric branch 3 times, most recently from 9d490fc to 04046f0 Compare February 12, 2020 00:46
stats/record_test.go Outdated Show resolved Hide resolved
stats/view/worker.go Outdated Show resolved Hide resolved
stats/record.go Outdated
@@ -31,10 +31,17 @@ func init() {
}
}

// Recorder is a subset of the view.Meter interface which only includes
// the Record method, to avoid circular imports between stats and view.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep this definition and simply include this in the Meter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I updated the comment, too.

What do you think of changing the second argument to Record to be []stats.Measurement rather than the current interface{}?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done.

Copy link
Contributor

@rghetia rghetia Feb 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of changing the second argument to Record to be []stats.Measurement rather than the current interface{}?

that is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out that this introduces an import cycle:

import cycle not allowed
 package go.opencensus.io/stats/view
	imports go.opencensus.io/stats
	imports go.opencensus.io/stats/internal
	imports go.opencensus.io/stats

Would you prefer that I move internal.DefaultRecorder out of internal, or do something like create an InternalMeasurement struct or interface and embed it in stats.Measurement?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say leave it as interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out that stats.Measurement.Measure() returns a stats.Measure, which seemed like a bridge too far in terms of putting things into Internal.

Take a look at 6003f86 for what that change would look like. I'm slightly concerned about moving DefaultRecorder from the internal package (and making it more public/obvious).

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted this to stick to interfaces.

stats/record.go Outdated Show resolved Hide resolved
stats/view/worker.go Outdated Show resolved Hide resolved
stats/record.go Outdated
@@ -58,6 +65,14 @@ func WithMeasurements(measurements ...Measurement) Options {
}
}

// WithMeter records the measurements to the specified `view.Meter`, rather
// than to the global metrics recorder.
func WithMeter(meter Recorder) Options {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance of RecordWithOptions WithMeter is 2/3rd of RecordWithOptions without meter

BenchmarkRecord8WithMeter-8      	  241356	       270 ns/op	     400 B/op	       4 allocs/op
BenchmarkRecord8WithoutMeter-8   	  335601	       197 ns/op	     368 B/op	       3 allocs/op

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the following, I get 231 ns/op, rather than 270 ns/op:

func BenchmarkRecord8_WithRecorder(b *testing.B) {
	ctx := context.Background()
	meter := view.NewMeter()
	meter.Start()
	defer meter.Stop()
	b.ResetTimer()

	for i := 0; i < b.N; i++ {
		stats.RecordWithOptions(ctx, stats.WithRecorder(meter), stats.WithMeasurements(m.M(1), m.M(1), m.M(1), m.M(1), m.M(1), m.M(1), m.M(1), m.M(1)))
	}

	b.StopTimer()
}

For the _Parallel case, I get 117 ns/op vs 108 ns/op:

goos: windows
goarch: amd64
pkg: go.opencensus.io/stats
BenchmarkRecord0-16                             12499986                98.0 ns/op
BenchmarkRecord1-16                              8571868               139 ns/op
BenchmarkRecord8-16                              6417102               188 ns/op
BenchmarkRecord8_WithRecorder-16                 5194611               231 ns/op
BenchmarkRecord8_WithRecorder_Parallel-16       10085448               117 ns/op
BenchmarkRecord8_Parallel-16                    11215130               108 ns/op
BenchmarkRecord8_8Tags-16                        5940622               198 ns/op

I can avoid the extra allocation and make the time between BenchmarkRecord8_WithRecorder and BenchmarkRecord8 match by pre-creating the stats.WithRecorder(meter) object like so:

rec := stats.WithRecorder(meter)

for (....) {
  stats.RecordWithOptions(ctx, rec, stats.WithMeasurements(....)
}

I note that the current benchmark doesn't perform any aggregation, so it's actually testing the shortcut path AFAICT. Adding a view which is tracking a simple Sum (no tags) increases the benchmark time like so:

BenchmarkRecord8_Aggregated-16                   1000000              1017 ns/op

Note that using a view here also affects any subsequent benchmarks for the same measurement, because the m.desc.subscribed() check around line 120 of record.go will always return true once it has been registered once.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BenchmarkRecord8_WithRecorder and BenchmarkRecord8 match by pre-creating the stats.WithRecorder(meter) object like so:

rec := stats.WithRecorder(meter)

for (....) {
  stats.RecordWithOptions(ctx, rec, stats.WithMeasurements(....)
}

+1.

Signed-off-by: Evan Anderson <evan.k.anderson@gmail.com>
Signed-off-by: Evan Anderson <evan.k.anderson@gmail.com>
Copy link
Contributor Author

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also added a benchmark in stats/view to show the overhead of going through the stats interface vs going directly through the worker backend commands:

pkg: go.opencensus.io/stats/view
BenchmarkRecordReqCommand-16              562048              2155 ns/op             288 B/op         17 allocs/op
BenchmarkRecordViaStats-16                461568              2401 ns/op             496 B/op         21 allocs/op

Note that this is with 8 tags, 2 of which are aggregated on, into 10 buckets. This did cache the WithRecorder option, so the 4 allocations are probably for the options object, the measurements object, the tag object, and the attachments.

@evankanderson
Copy link
Contributor Author

(Let me know if you want me to squash the commits or if there are other changes I need to make.)

@rghetia
Copy link
Contributor

rghetia commented Feb 14, 2020

(Let me know if you want me to squash the commits or if there are other changes I need to make.)

@evankanderson
Leave it as is. When I merge, I will squash it.

@songy23 would have time to review this? I would like to have one more reviewer.

Copy link
Contributor

@songy23 songy23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The high level idea makes sense to me. The only alternative I can think of is to hard-code resources as metric labels, export to Collector, then have some sort of metric processor on Collector to reconstruct resources and strip those labels. For example, in addition to normal metric labels, add labels ["container_name": "some container", "pod_name": "pod"], then on the Collector remove those labels and attach a new k8s resource with these values.

We have internal use cases like this, but I'm don't think that's ideal since it requires lots of hard code and also mixes resources and metrics in a confusing way.

@evankanderson
Copy link
Contributor Author

I have code like that, too.

Worse than that, I have code which auto-detects which type of resource it is, and has a fixed schema for each Resource of which tags should be extracted and removed from the tag map.

I suppose that I could still go through tags for aggregation and add a "RegisterType" method into my exports which would allow dynamically registering the schemas, but misdirecting all the aggregation through multiple layers of Tags and view aggregations feels wrong to me.

@rghetia
Copy link
Contributor

rghetia commented Feb 18, 2020

The high level idea makes sense to me. The only alternative I can think of is to hard-code resources as metric labels, export to Collector, then have some sort of metric processor on Collector to reconstruct resources and strip those labels. For example, in addition to normal metric labels, add labels ["container_name": "some container", "pod_name": "pod"], then on the Collector remove those labels and attach a new k8s resource with these values.

We have internal use cases like this, but I'm don't think that's ideal since it requires lots of hard code and also mixes resources and metrics in a confusing way.

I agree that it would be confusing. Even though OC will be deprecated once Otel is released, this PR probably makes more sense than the alternate solution. It is also inline with concept of Meter in Otel. Do you agree?

Copy link
Contributor

@songy23 songy23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but misdirecting all the aggregation through multiple layers of Tags and view aggregations feels wrong to me.

It is also inline with concept of Meter in Otel. Do you agree?

+1 to both. This approach looks much cleaner than the alternative.

@rghetia rghetia merged commit 84d38db into census-instrumentation:master Feb 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support exporting different stats to different exporters.
4 participants