census-instrumentation / opencensus-go Goto Github PK

AggregationDistributionValue and AggregationCountValue are intended for internal testing purposes. Unexport them until end users require them for white box testing.

/cc @bogdandrutu @hyangah

Aggregation implementations are using "Aggregation" as a prefix, Window implementations are using "Window" as a prefix. In Go, we prefer the natural language order.

Some example type names with the new order:

type CountAggregation struct{}
type DistributionAggregation struct{}

type SlidingTimeWindow struct {}
type SlidingCountWindow struct {}

Consider the following view:

        // Create view to see the processed video size cumulatively.
	view := stats.NewView(
		"my.org/views/video_size_cum",
		"processed video size over time",
		nil,
		videoSize,
		stats.DistributionAggregation([]float64{0, 1 << 16, 1 << 32}),
		stats.Cumulative{},
	)

While exporting, it errors with:

Wait longer than the reporting duration...
2017/11/21 13:45:34 Failed to export to Stackdriver Monitoring: rpc error: code = InvalidArgument desc = Field timeSeries[0].points[0].distributionValue.range is not allowed: Distribution range is not yet supported.

setReportingPeriodReq.handleCommand needs to send to channel cmd.c in the case where cmd.d <= 0.

We should either use singular of plural. exporter subdirectory is singular, plugins is plural.

/cc @odeke-em

gRPC and AppEngine requires 1.6 support to be around for a little while. Support 1.6 by removing the sort.Slice and context package dependencies.

See also grpc/grpc-go#1603.

We would like to avoid users accidentally dropping the tags in the current context. Make NewMap working against a context.Context rather than an initial tag.Map.

func NewMap(ctx context.Context, m ...Mutator)

• Please add a file called AUTHORS with one line for the moment Google Inc.
• Change all licenses headers to: Copyright [YEAR], OpenCensus Authors

See census-instrumentation/opencensus-java#539

The encoder and decoders currently allocate and give users no option to reuse the same buffer. Instead of the current Encode and Decode function, provide the following:

func Encode(w io.Writer, ts *TagSet)
func Decode(r io.Reader) *TagSet

There are lightweight types such as WindowSlidingTime, WindowSlidingCount, and WindowCumulative that are not complicated enough to require a constructor. Remove NewWindowCumulative and make the zero value useful, remove NewWindowSlidingCount and export fields on WindowSlidingCount, and remove NewWindowSlidingTime and export the fields of WindowSlidingTime.

/cc @hyangah @bogdandrutu

Build is broken because golint is not available, see an example log at https://travis-ci.org/census-instrumentation/opencensus-go.

Provide a stats handler that does enables both stats and tracing. Users who prefer not to have both can depend on grpcstats and grpctrace packages individually.

Currently, in the stats package:

• Views are required to be initialized, registered and subscribed to in order to collect data via exporters.
• Views are also automatically registered if they are subscribed to after initialization.
• Views are required to be initialized, registered and force collected in order to collect data via force collection.

This is a complicated sequence of actions required from the user and it is easy to get it wrong. It seems like registration can be handled as a part of subscription and force collection.

I am suggesting the following sequence instead:

• Views are required to be initialized and subscribed to in order to collect data. If a view with the same name exists, subscription returns an error. Unsubscribe to free the name.
• Views are required to be initialized and force collected from in order to collect data. If a view with the same name exists, force collection API returns an error. Stop force collection to free the name.

Thoughts?

/cc @dinooliva @bogdandrutu

Force collection became identical to the subscription after subscription via channels is removed. Remove force collection related APIs.

stats.Measurement is a value with a defined set of types, so it should be defined as a struct that can only be constructed inside the stats package, not as an interface with an unexported method.

Sketch:

type Measurement struct {
  m interface{} // one of *Float64Measure or *Int64Measure
  v interface{} // one of int64 or float64
}

ToOrderedTagsSlice can be redesigned into:

func DecodeOrderedTags(buf []byte, keys []Key) []Tag

ToValuesString can be redesigned into:

func EncodeOrderedTags(set *Set, keys []Key) []byte

We should also consider whether we need to provide these utilities from the tags package.

ContainsRow and EqualRows are both utilities to support the stats package tests. Hide them from the user, given they will never be required by the end user.

User input need to be validated whether they are fitting the restrictions we set. Use the Java client as a reference to validate the input.

As well as aggregation data types:

type MeanData float64
type SumData float64

To be consistent with the other language implementations.

Accessors don't need a Get prefix and without the prefix it is consistent with tags.KeyStringByName.

rpcData.reqCount and rpcData.respCount are written to atomically but not read atomically, which doesn't seem right.

func (k *KeyString) ValueAsString(b []byte) string

is not accessing any of the Key fields and is doing a redundant job of casting a []byte into string.

/cc @acetechnologist

From #41, the section about "Recording Measurements" in the docs doesn't have much to help the reader understand when to record a bunch of measurements together versus when to record a measurement individually.

Currently the stats API contains quite a few package-level APIs works primarily with a View instance.

func RegisterView(v View) error
func RetrieveData(v View) ([]*Row, error)
func StopForcedCollection(v View) error
func SubscribeToView(v View, c chan *ViewData) error
func UnregisterView(v View) error
func UnsubscribeFromView(v View, c chan *ViewData) error

They could instead accept a View as a receiver. There are two advantages: (a) some function names will be shorter because we will be able to drop the "View" suffix from their names, (b) godoc will render them under the View type and it is easy for their discoverability.

func (v View) Register() error
func (v View) RetrieveData() ([]*Row, error)
func (v View) StopForcedCollection() error
func (v View) Subscribe(c chan *ViewData) error
func (v View) Unregister() error
func (v View) Unsubscribe(c chan *ViewData) error

Happy to send a PR.

/cc @acetechnologist @bogdandrutu

All measure types need to be able to report their unit. Enforce measures to return a unit as string.

type Measure interface {
     Name() string
     Unit() string
}

Even though CreateKeyString is a constructor, it doesn't follow the conventional NewX naming.

/cc @acetechnologist

Go provides testable examples. Move the snippets to the godoc examples. In the alpha stage, while the APIs are being broken often, it will help us to keep the examples up to date.

Add a pprof.Do like utility to record profiler samples with a given tag map. Do looks for the tags inside the given context and sets them as pprof labels. Any goroutine started in f needs to propagate the tag map via c in order to be able to keep setting the labels in the new goroutines.

func Do(ctx context.Context, m *Map, f func(c context.Context))

Thanks @bogdandrutu for catching this.

For the Stackdriver Stats exporter, the name of MetricDescriptor needs to be under "custom.googleapis.com/opencensus/". The format of the whole name should be String.format("projects/%s/metricDescriptors/custom.googleapis.com%%2Fopencensus%%2F%s", projectId, viewName.replace("/", "%2F")). Stackdriver might return a permission denied error if we don't follow the name convention.

See https://github.com/census-instrumentation/opencensus-java/pull/827/files#diff-d2886f5b11a5e3be7a00a8f5fff5a1f0R84 for changes to Java exporter. Also refer to https://cloud.google.com/monitoring/custom-metrics/creating-metrics#custom_metric_names.

tags.TagSet is repeating the "tag" twice and we don't like to stuttering in Go. Rename it to Set.

DecodeOrderedTags and EncodeOrderedTags are both used to generate signatures for the stats collectors. Both are useless for the end user, move them to stats package to keep the API surface clean.

(I already have a PR for this that depends on #98.)

Consider the following view:

        view := stats.NewView(
		"my.org/views/video_size_cum",
		"processed video size over time",
		nil,
		videoSize,
		stats.CountAggregation{},
		stats.Cumulative{},
	)

It fails with:

2017/11/21 14:52:52 Failed to export to Stackdriver Monitoring: rpc error: code = InvalidArgument desc = Request was missing field timeSeries[0].points[0].distributionValue.

Currently, custom delta metric descriptions cannot be created via the Stackdriver Monitoring API. Be consistent with the Java exporter and avoid creating/uploading data if the window is not cumulative. See https://github.com/census-instrumentation/opencensus-java/pull/713/files#diff-d2886f5b11a5e3be7a00a8f5fff5a1f0R73.

It's unclear whether tags.Set is intended to be mutable or immutable. @rakyll mentioned wanting to hide the encoding details from the user, so I recommended the following:

type Set map[Key]Value

func (set Set) Copy() Set

type Value struct {
  b []byte
}

func StringValue(s string) Value {
  return Value{[]byte(s)}
}

func Int64Value(i int64) Value {
  return Value{[]byte{/* encode the int */}}
}

Rather than allowing users access to the low-level buffer, introduce exporters and report the view data via the registered ones.

// Exporter exports the collected records as view data.
type Exporter interface {
	Export(viewData *ViewData)
}

// RegisterExporter registers an exporter.
func RegisterExporter(name string, e Exporter)

// UnregisterExporter unregisters an exporter.
func UnregisterExporter(name string)

As a follow up, we also need a rethinking of the current subscription model. In order to access to the view data of a view, you currently need to subscribe to a view after it is registered globally. Regardless of the registration status, view data can still be collected by force collection. Instead of having both methods available, we should either allow users to force collect or subscribe but not both.

It is not clear whether it is legal to use RetrieveData to retrieve the collected data and exporters at the same time. Document whether they can be mutually used.

To be more consistent with the Java library, rename AggregationValue to AggregationData.

All types named after AggregationValue will also be renamed:

• CountAggregationValue -> CountData
• DistributionAggregationValue -> DistributionData

The trace package currently allows users to print in a few different ways:

func LazyPrint(ctx context.Context, str fmt.Stringer)
func LazyPrintWithAttributes(ctx context.Context, attributes []Attribute, str fmt.Stringer)
func LazyPrintf(ctx context.Context, format string, a ...interface{})
func LazyPrintfWithAttributes(ctx context.Context, attributes []Attribute, format string, a ...interface{})
func Print(ctx context.Context, str string)
func PrintWithAttributes(ctx context.Context, attributes []Attribute, str string)

Instead of giving users different combinations of these options, we should provide a printing entry type and allow user to provide their options via the new type.

type Entry struct {
	Text       fmt.Stringer
	Attributes []Attribute
}

func LazyPrint(ctx context.Context, e Entry)

Then, we can provide maybe one other option for convience:

func LazyPrintf(ctx context.Context, format string, a ...interface{})

Users who doesn't want lazy formatting can always implement their own fmt.Stringer to format it at the point LazyPrint is called.

/cc @jcd2 @bogdandrutu

Add stats benchmarks, start with the gRPC benchmarks with the stats instrumentation turned on.

gRPC integration should use google.golang.org/grpc/grpclog instead of github.com/golang/glog.

All FindX functions are looking for an item in the map and return and error if not found. We should instead return a boolean for the convince of the user to handle nonexistence cases.

func FindMeasure(name string) (m Measure, ok bool)

func FindView(name string) (v *View, ok bool)

Metrics description creation is failing with "AlreadyExists" if the metric descriptor is being created for the first time:

Wait longer than the reporting duration...
2017/11/21 15:03:27 Failed to export to Stackdriver Monitoring: rpc error: code = AlreadyExists desc = Errors during metric descriptor creation: {(metric: custom.googleapis.com/opencensus%2Fmy.org%2Fviews%2Fvideoooosizeovertime, error: The metric already exists.)}.

Mutators and NewMap should validate the key values and return a validation errors if the input is not aligning with the validation rules from the spec.

See census-instrumentation/opencensus-specs#12 for more info.

Tags allow typed keys such (e.g. StringKey) but there is interest to drop typed keys and have StringKey as the only option. Determine the future for the tag package and remove the unnecessary abstractions before freezing the APIs.

/cc @dinooliva

Metrics are uploaded without the recorded tags being attached as Stackdriver Monitoring labels. Fix the exporter by adding the labels.

Both function names are mouthful, Go libraries often just use Encode to encode a concrete type to []byte, and Decode to unmarshal from a []byte.

/cc @acetechnologist

To reduce to dependencies to a specific git provider and to enhance the the current long and noisy import path, setup a vanity URL for the package and enforce the vanity URL everywhere. The vanity URL is either going to be "go.opencensus.io" or "opencensus.io/go".