Add support for reader. by rghetia · Pull Request #1049 · census-instrumentation/opencensus-go

rghetia · 2019-03-05T02:01:34Z

No description provided.

metric/reader/reader.go

metric/reader/reader_test.go

songy23 · 2019-03-06T17:24:31Z

metric/reader/reader.go

 // Reader periodically reads metrics from all producers registered
 // with producer manager and exports those metrics using provided
-// exporter. Call Reader.Stop() to stop the reader.
+// exporter1. Call Reader.Stop() to stop the reader1.


What are "exporter1" and "reader1"? Typo?

when refactored it renamed in reader.go as well. I'll fix it.

songy23

LGTM

rghetia · 2019-03-06T18:10:49Z

@bogdandrutu PTAL.

bogdandrutu · 2019-03-10T18:25:07Z

metric/reader/reader.go

+// limitations under the License.
+//
+
+package reader


Why is this not in metricexport? Also generic names are not good for go packages (so this should be metricreader), I forgot to mention this for producer that should be metricproducer (please fix that).

bogdandrutu · 2019-03-10T18:27:27Z

metric/registry.go

+	r.mu.Lock()
+	defer r.mu.Unlock()
 	existing, ok := r.gauges[name]
 	if ok {


Instead of log.Panic probably we should return an error. you can file an issue for this.

bogdandrutu · 2019-03-10T18:28:24Z

metric/registry.go

 // External synchronization is required if you want to add gauges to the same
 // registry from multiple goroutines.
 type Registry struct {
+	mu     sync.RWMutex


This is a different issue, can we split it?

rghetia · 2019-03-12T22:42:19Z

@bogdandrutu PTAL.

rghetia · 2019-03-13T17:22:18Z

metric/export.go

+
+// Exporter is an interface that exporters implement to export the metric data.
+type Exporter interface {
+	ExportMetric(data []*metricdata.Metric)


this conflicts with already defined interface used for OC-service.
Should this be changed to ?

Export( ) OR

ExportMetrics() . - It is plural, could be very confusing.

OR should the oc-service change it to

ExportMetricPb(...) - because it exports metric protobuf

@bogdandrutu, @songy23 any thoughts?

should the oc-service change it to

ExportMetricPb(...) - because it exports metric protobuf

I would suggest to go with this approach. I think we should stick to "Metric" when we're referring to the native Go metrics data model and "MetricPb" when referring to the proto-generated files.

/cc @odeke-em

rghetia · 2019-03-15T22:38:04Z

@songy23, @bogdandrutu PTAL.
Earlier Reader was actually Interval Reader. So renamed it to IntervalReader. Added Reader. It is similar to java now. Prometheus will just use Reader while Stackdriver will user IntervalReader and Reader.

songy23

LGTM

bogdandrutu · 2019-03-18T23:53:23Z

metric/metricexport/reader.go

+}
+
+// Options to configure optional parameters for Reader.
+type Options struct {


Options is very generic name to have in this package. Consider to use the same pattern as https://golang.org/src/net/http/server.go?s=76578:79938#L2468

So you have a IntervalReader struct with exported properties like ReportingInterval, users will have to set this before calling "Start"

intervalReader := NewIntervalReader(reader, exporter) intervalReader.ReportingInterval = 30 * time.Second intervalReader.Start()

You can check the reporting interval at the beginning of Start and save it in a local variable.

bogdandrutu · 2019-03-19T00:01:37Z

metric/metricexport/reader.go

+// with producer manager and exports those metrics using provided
+// exporter.
+type Reader struct {
+	Sampler trace.Sampler


No need to export this for the moment.

bogdandrutu · 2019-03-19T00:04:54Z

metric/export.go

+)
+
+// Exporter is an interface that exporters implement to export the metric data.
+type Exporter interface {


Why is this in the metric package and not metricexport?

rghetia · 2019-03-19T06:27:38Z

@bogdandrutu PTAL.

bogdandrutu · 2019-03-19T19:25:22Z

metric/metricexport/reader.go

+	}
+
+	ctx := context.Background()
+	_, span := trace.StartSpan(


This probably should be:

ctx, span := trace.StartSpan(context.Background(), spanName, trace.WithSampler(...))

bogdandrutu · 2019-03-19T19:26:32Z

metric/metricexport/reader.go

+		data = append(data, producer.Read()...)
+	}
+	// TODO: [rghetia] what to do with errors? log them?
+	exporter.ExportMetrics(ctx, data)


We need to record metrics for this. Not now.

bogdandrutu · 2019-03-19T22:49:13Z

metric/metricexport/reader.go

+// Reader reads metrics from all producers registered
+// with producer manager and exports those metrics using provided
+// exporter.
+type Reader struct {


Probably for this we should do a WithOption pattern because does not have a start method.

…r is stopped.

Also include ctx in ExportMetric api.

rghetia · 2019-03-21T04:51:52Z

@bogdandrutu PTAL.

* Add support for reader. * handle multiple call to reader.Stop and test for producer while reader is stopped. * fix typo. * fix review comment and refactor reader to metricexport package. * change Reader to IntervalReader and add Reader. Also include ctx in ExportMetric api. * modify export interface to return error and make it plural. * remove option and provide Start function. * move exporter.go to metricexport package. * added option for reader. * make constants private. (cherry picked from commit 5ae9166)

rghetia requested a review from rakyll as a code owner March 5, 2019 02:01

rghetia requested review from bogdandrutu and songy23 March 5, 2019 06:44

songy23 reviewed Mar 5, 2019

View reviewed changes

metric/reader/reader.go Outdated Show resolved Hide resolved

metric/reader/reader_test.go Outdated Show resolved Hide resolved

songy23 reviewed Mar 6, 2019

View reviewed changes

songy23 approved these changes Mar 6, 2019

View reviewed changes

bogdandrutu reviewed Mar 10, 2019

View reviewed changes

rghetia force-pushed the reader branch from a1b756c to 9e0ab83 Compare March 12, 2019 21:33

rghetia requested a review from a team as a code owner March 12, 2019 21:33

rghetia commented Mar 13, 2019

View reviewed changes

rghetia mentioned this pull request Mar 15, 2019

Change ExportMetric api to ExportMetricProto census-ecosystem/opencensus-go-exporter-stackdriver#106

Merged

rghetia force-pushed the reader branch from 9e0ab83 to 791f5da Compare March 15, 2019 22:18

songy23 approved these changes Mar 16, 2019

View reviewed changes

bogdandrutu reviewed Mar 19, 2019

View reviewed changes

rghetia mentioned this pull request Mar 19, 2019

add support for exporting metrics data model census-ecosystem/opencensus-go-exporter-stackdriver#109

Merged

bogdandrutu reviewed Mar 19, 2019

View reviewed changes

rghetia added 9 commits March 20, 2019 12:33

Add support for reader.

da7739e

handle multiple call to reader.Stop and test for producer while reade…

b0aacd1

…r is stopped.

fix typo.

8021542

fix review comment and refactor reader to metricexport package.

8b8700c

change Reader to IntervalReader and add Reader.

b3bad5a

Also include ctx in ExportMetric api.

modify export interface to return error and make it plural.

426ab1b

remove option and provide Start function.

10e804d

move exporter.go to metricexport package.

474ba75

added option for reader.

2685fd0

rghetia force-pushed the reader branch from 196cc3a to 2685fd0 Compare March 20, 2019 19:45

make constants private.

e8055b9

bogdandrutu approved these changes Mar 22, 2019

View reviewed changes

rghetia merged commit 5ae9166 into census-instrumentation:master Mar 22, 2019

rghetia mentioned this pull request Mar 22, 2019

Export Metrics instead of ViewData. #956

Closed

rghetia deleted the reader branch April 15, 2019 20:43

Conversation

rghetia commented Mar 5, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

songy23 left a comment

Choose a reason for hiding this comment

Uh oh!

rghetia commented Mar 6, 2019

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!

rghetia commented Mar 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

songy23 Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rghetia commented Mar 15, 2019

Uh oh!

songy23 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!

rghetia commented Mar 19, 2019

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!

rghetia commented Mar 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

songy23 Mar 13, 2019 •

edited

Loading