Add GetMonitoredResource option #46

semistrict · 2018-09-27T22:46:49Z

This allows customizing the MonitoredResource based on tag values.

bogdandrutu · 2018-10-04T02:43:41Z

stackdriver.go

+	//
+	// The MonitoredResource field is ignored if this field is set to a non-nil
+	// value.
+	GetMonitoredResource func([]tag.Tag) monitoredresource.Interface


Should this take an ViewData and also return a new ViewData and MonitorResource. User usually want to remove that tag probably.

Also some people want to configure the project id as well. This is useful when export metrics on behalf of others.

I made it take and return a []tag.Tag since I think this will accomplish the same thing without the complexity of having to clone and mutate a full ViewData. Regarding the project ID, it is part of the MonitoredResource.

semistrict · 2018-10-04T16:31:18Z

monitoredresource/monitored_resources_test.go

@@ -101,11 +101,3 @@ func TestAWSEC2InstanceMonitoredResources(t *testing.T) {
 		t.Errorf("AWSEC2InstanceMonitoredResource Failed: %v", autoDetected)
 	}
 }
-
-func TestNullMonitoredResources(t *testing.T) {


This was failing on Travis because (I guess) the Travis worker is an EC2 instance.

It's actually a GCE instance :) Tests failed because GCE resource is detected correctly, which is a good indication.

semistrict · 2018-10-04T16:32:06Z

stats_test.go

@@ -1105,7 +1264,7 @@ func TestExporter_customContext(t *testing.T) {
 	var timedOut = 0
 	createMetricDescriptor = func(ctx context.Context, c *monitoring.MetricClient, mdr *monitoringpb.CreateMetricDescriptorRequest) (*metric.MetricDescriptor, error) {
 		select {
-		case <-time.After(15 * time.Millisecond):
+		case <-time.After(1 * time.Second):


Increase time because it was failing intermittently on Travis

semistrict · 2018-10-04T16:33:29Z

@bogdandrutu updated PTAL

rghetia · 2018-10-05T15:56:41Z

stackdriver.go

+
+	// GetMonitoredResource may be provided to supply the details of the
+	// monitored resource dynamically based on the tags associated with each
+	// data point. Most users will not need need to set this, but should instead


nit:s/need need/need/

rghetia · 2018-10-05T16:40:25Z

stackdriver.go

+	//
+	// The MonitoredResource field is ignored if this field is set to a non-nil
+	// value.
+	GetMonitoredResource func([]tag.Tag) ([]tag.Tag, monitoredresource.Interface)


Is the purpose here to create monitored resource labels based only on Tags? OR will it be a combination of two (based on tag and GCEInstance/GKE/AWS)?
Since MonitoredResource and GetMonitoredResource both cannot be set and GetMonitoredResource supersedes MonitoredResource, can MonitoredResource be deprecated?
we also have this PR on monitored resources. We would have three different ways to create monitored resources.

/cc @rakyll

I don't think MonitoredResource should be deprecated even though there is a lot of overlap in functionality. GetMonitoredResources is really an advanced feature that most users should not need to care about. MonitoredResource on the other hand should usually be set by most users and is much easier to understand.

semistrict · 2018-10-06T12:40:39Z

@bogdandrutu updated to allow modification of the tags, PTAL

This allows customizing the MonitoredResource based on tag values.

semistrict · 2018-10-17T16:50:22Z

ping @bogdandrutu

songy23

This change LGTM though I don't have much context about the background. /cc @fabxc.

songy23 · 2018-10-22T16:13:35Z

monitoredresource/monitored_resources_test.go

@@ -101,11 +101,3 @@ func TestAWSEC2InstanceMonitoredResources(t *testing.T) {
 		t.Errorf("AWSEC2InstanceMonitoredResource Failed: %v", autoDetected)
 	}
 }
-
-func TestNullMonitoredResources(t *testing.T) {


It's actually a GCE instance :) Tests failed because GCE resource is detected correctly, which is a good indication.

fabxc · 2018-10-30T14:45:43Z

I wanted to submit a PR with support for resources and this certainly conflicts.

@bogdandrutu @Ramonza could you provide some background on the use case this was added for? I'd like to find a good way to accommodate this use case within the new resources.

semistrict · 2018-10-30T17:16:36Z

@fabxc this was added so that you can record against more than one monitored resource based on what tags are set. For example, if you are recording stats on behalf of another process.

fabxc · 2018-10-31T10:01:46Z

We deliberately deferred the discussion of multiple resources from a single process initially. I thought this would rather be an aspect of the core library as well and would either way be discussed in the scope of a language and exporter independent spec.
Solving it at the level of an individual exporter seems to invite a lot of fragmented and incompatible approaches. This particular implementation states that view and tags can mutate the resources, which has implications that I'd have liked to see discussed.

The only way forward right now seems to be to provide the intended core resource handling as a GetMonitoredResource function that ignores the tags. I strongly dislike this since the exporter by default effectively ignores the core resource concept.
I also thought the monitoredresource package would become deprecated entirely, since the actual logic in there was factored out to census-ecosystem/opencensus-go-resource. But due to the monitoredresource.Interface type it would now be around forever.

@bogdandrutu

googlebot added the cla: yes label Sep 27, 2018

semistrict force-pushed the customize-monitoredres branch 7 times, most recently from e5e6bd7 to 82e3217 Compare October 4, 2018 00:44

semistrict requested a review from rghetia October 4, 2018 00:59

bogdandrutu reviewed Oct 4, 2018

View reviewed changes

semistrict force-pushed the customize-monitoredres branch 2 times, most recently from 18bf666 to 1eb1b2e Compare October 4, 2018 16:30

semistrict commented Oct 4, 2018

View reviewed changes

rghetia reviewed Oct 5, 2018

View reviewed changes

semistrict force-pushed the customize-monitoredres branch from 1eb1b2e to 8d57087 Compare October 6, 2018 12:40

Add GetMonitoredResource option

7baa60f

This allows customizing the MonitoredResource based on tag values.

semistrict force-pushed the customize-monitoredres branch from 8d57087 to 7baa60f Compare October 6, 2018 12:44

semistrict assigned bogdandrutu Oct 6, 2018

semistrict assigned songy23 Oct 22, 2018

songy23 approved these changes Oct 22, 2018

View reviewed changes

semistrict merged commit f6033e3 into census-ecosystem:master Oct 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GetMonitoredResource option #46

Add GetMonitoredResource option #46

semistrict commented Sep 27, 2018

bogdandrutu Oct 4, 2018

bogdandrutu Oct 4, 2018

semistrict Oct 4, 2018

semistrict Oct 4, 2018

songy23 Oct 22, 2018

semistrict Oct 4, 2018

semistrict commented Oct 4, 2018

rghetia Oct 5, 2018

rghetia Oct 5, 2018

rghetia Oct 5, 2018

semistrict Oct 6, 2018

semistrict commented Oct 6, 2018

semistrict commented Oct 17, 2018

songy23 left a comment

songy23 Oct 22, 2018

fabxc commented Oct 30, 2018

semistrict commented Oct 30, 2018

fabxc commented Oct 31, 2018 •

edited

Loading

Add GetMonitoredResource option #46

Add GetMonitoredResource option #46

Conversation

semistrict commented Sep 27, 2018

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

semistrict commented Oct 4, 2018

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

semistrict commented Oct 6, 2018

semistrict commented Oct 17, 2018

songy23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabxc commented Oct 30, 2018

semistrict commented Oct 30, 2018

fabxc commented Oct 31, 2018 • edited Loading

fabxc commented Oct 31, 2018 •

edited

Loading