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

Check if Stackdriver MetricDescriptor is compatible with the view #162

Merged
merged 1 commit into from
Nov 28, 2017

Conversation

rakyll
Copy link
Contributor

@rakyll rakyll commented Nov 27, 2017

Fixes #146.

a = &stats.DistributionAggregation{}
}

if reflect.TypeOf(agg) != reflect.TypeOf(a) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should also check whether agg is a pointer or not, or disallow concrete types to be passed as aggregators.

if reflect.TypeOf(agg) != reflect.TypeOf(a) {
return false
}
if reflect.TypeOf(window) != reflect.TypeOf(w) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto here.

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.

Looks good, though I think we also need to check the label descriptors of MetricDescriptor if it exisits. Uploading TimeSeries with missing labels will also introduce an error.

@jcd2 jcd2 self-requested a review November 28, 2017 00:47
@rakyll
Copy link
Contributor Author

rakyll commented Nov 28, 2017

Added the labels, and addressed the concrete type vs pointers.

@@ -189,26 +191,36 @@ func (e *Exporter) makeReq(vds []*stats.ViewData) *monitoringpb.CreateTimeSeries
}
}

// createMeasure creates a MetricDescriptor for the given view data in Stackdriver Monitoring.
// If there is already a metric descriptor created with the same name
// and the metric descriptor has a different aggregation/window, it returns an error.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: aggregation/window -> aggregation/window/tag keys

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

_, ok := e.createdViews[viewName]
if ok {
if md, ok := e.createdViews[viewName]; ok {
// Check agg and window.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: and tag keys

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

@rakyll rakyll merged commit 5f53bd7 into census-instrumentation:master Nov 28, 2017
@rakyll rakyll deleted the sda branch November 28, 2017 02:43
})
if err := equalAggWindowTagKeys(md, agg, window, tagKeys); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This call to equalAggWindowTagKeys should be moved inside the "if err == nil" block below.

Copy link
Member

Choose a reason for hiding this comment

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

I'll complete this part of the review @jcd2, if it is okay with you @rakyll.

Copy link
Member

Choose a reason for hiding this comment

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

This was fixed by @songy23 in PR #205 so it is all done.

bogdandrutu pushed a commit to bogdandrutu/opencensus-go that referenced this pull request Feb 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants