Add resource package #878

fabxc · 2018-08-22T16:54:26Z

This package adds resources as a core concept to OpenCensus.

@rghetia @bogdandrutu @Ramonza @rakyll @jkohen As this is the core step of a relatively fundamental change, you may all want to take a quick look.

This package adds resources as a core concept to OpenCensus. Signed-off-by: Fabian Reinartz <[email protected]>

semistrict · 2018-08-22T16:58:25Z

@fabxc this is an interesting idea but definitely needs to be discussed on opencensus-specs first, would you mind raising an issue or PR there for discussion?

rakyll

Leaving some high-level readability comments, haven't reviewed the encoder/decoder implementations.

rakyll · 2018-08-22T21:41:55Z

resource/resource_test.go

+			},
+		},
+	}
+	for _, c := range cases {


Use t.Run to create sub tests, so they can be individually run and output is more readable.

rakyll · 2018-08-22T21:42:11Z

resource/resource_test.go

+		{s: `a, extra=chars`, fail: true},
+	}
+	for i, c := range cases {
+		t.Logf("test %d: %s", i, c.s)


Ditto, don't Log. Use t.Run.

rakyll · 2018-08-22T21:42:34Z

resource/resource_test.go

+
+func TestMerge(t *testing.T) {
+	cases := []struct {
+		a, b, expect *Resource


s/expect/want

rakyll · 2018-08-22T21:43:35Z

resource/resource_test.go

+
+func TestDecodeTags(t *testing.T) {
+	cases := []struct {
+		s      string


s/s/encoded

rakyll · 2018-08-22T21:43:40Z

resource/resource_test.go

+func TestDecodeTags(t *testing.T) {
+	cases := []struct {
+		s      string
+		expect map[string]string


rakyll · 2018-08-22T21:48:38Z

resource/resource.go

+}
+
+// FromEnvVars loads resource information from the OC_TYPE and OC_RESOURCE_TAGS environment variables.
+func FromEnvVars(context.Context) (*Resource, error) {


We can call this just FromEnv and dropping the var, similar to the standard library style: https://golang.org/pkg/os/#Setenv.

resource/resource.go

rakyll · 2018-08-22T21:52:23Z

resource/resource.go

+// If a value contains whitespaces, =, or " characters, it must always be quoted.
+var tagRegex = regexp.MustCompile(`\s*([a-zA-Z0-9-_./]+)=(?:(".*?")|([^\s="]+))\s*,`)
+
+func DecodeTags(s string) (map[string]string, error) {


rakyll · 2018-08-22T21:52:39Z

resource/resource.go

+	return s
+}
+
+// We accept domain names and paths as tag keys. Values may be quoted or unquoted in general.


Document this restriction somewhere in the godoc.

rakyll · 2018-08-22T21:55:01Z

resource/resource.go

+}
+
+// EncodeTags encodes a tags to a string as provided via the OC_RESOURCE_TAGS environment variable.
+func EncodeTags(tags map[string]string) (s string) {


Don't have named return variables if name is not self documenting.

s/s string/string

Signed-off-by: Fabian Reinartz <[email protected]>

rghetia · 2018-09-12T17:11:44Z

resource/resource_test.go

+		{encoded: `missing=leading-quote"`, wantFail: true},
+		{encoded: `extra=chars, a`, wantFail: true},
+		{encoded: `a, extra=chars`, wantFail: true},
+		{encoded: `a, extra=chars`, wantFail: true},


rghetia · 2018-09-12T18:15:05Z

resource/resource.go

+
+// Detectall calls all input detectors sequentially an merges each result with the previous one.
+// It returns on the first error that a sub-detector encounters.
+func DetectAll(ctx context.Context, detectors ...Detector) (*Resource, error) {


Does DetectAll need to be public?

I'd say so. Seems like a useful helper function that library users may inevitably rebuild for testing or similar.

ChainedDetector(d1, d2, ...)(ctx) does the same. why do we need two ways?

ChainedDetector is implemented using DetectAll (:

Keep the method just make it private detectAll().

resource/resource_test.go

fabxc · 2018-09-13T10:19:53Z

Can someone take a look at the appveyor build failure? It doesn't seem like it's related to the code:

go test -race -v .\...
?   	go.opencensus.io	[no test files]
?   	go.opencensus.io/examples/exporter	[no test files]
# runtime/cgo
exec: "gcc": executable file not found in %PATH%
FAIL	go.opencensus.io/exporter/jaeger [build failed]
FAIL	go.opencensus.io/exporter/prometheus [build failed]
FAIL	go.opencensus.io/exporter/stackdriver/propagation [build failed]

Signed-off-by: Fabian Reinartz <[email protected]>

rghetia · 2018-09-13T16:28:15Z

@fabxc ignore the appveyor build failure. (see #894 (comment))

Signed-off-by: Fabian Reinartz <[email protected]>

rakyll · 2018-09-22T00:19:14Z

/cc @Ramonza can you review?

semistrict · 2018-09-14T21:14:16Z

resource/resource.go

+	envVarTags = "OC_RESOURCE_TAGS"
+)
+
+// Resource describes an entity about which data is exposed.


Can we be more specific than "data"?

Maybe give some examples

semistrict · 2018-09-14T21:14:46Z

resource/resource.go

+	Tags map[string]string
+}
+
+// EncodeTags encodes a tags to a string as provided via the OC_RESOURCE_TAGS environment variable.


encodes a tags map?

semistrict · 2018-09-14T21:15:07Z

resource/resource.go

+
+var tagRegex = regexp.MustCompile(`\s*([a-zA-Z0-9-_./]+)=(?:(".*?")|([^\s="]+))\s*,`)
+
+// DecodeTags decodes serialized a tag map as used in the OC_RESOURCE_TAGS variable.


decodes a serialized tag map

semistrict · 2018-09-14T21:22:55Z

resource/resource.go

+	return m, nil
+}
+
+// FromEnv loads resource information from the OC_TYPE and OC_RESOURCE_TAGS environment variables.


s/OC_TYPE/OC_RESOURCE_TYPE/

semistrict · 2018-09-22T03:26:59Z

resource/resource.go

+// FromEnv loads resource information from the OC_TYPE and OC_RESOURCE_TAGS environment variables.
+func FromEnv(context.Context) (*Resource, error) {
+	res := &Resource{
+		Type: strings.TrimSpace(os.Getenv(envVarType)),


What happens if this env var is not set?

It will be the empty string, i.e. equivalent to net setting Type. If it doesn't get populated later on, e.g. by the agent, exporters can decide how to handle this.

semistrict · 2018-09-22T03:28:49Z

resource/resource.go

+	if res.Type == "" {
+		res.Type = b.Type
+	}
+	for k, v := range b.Tags {


If you first added all the b tags and then the a tags then you wouldn't need to check for the existence of the key in the map before overwriting.

semistrict · 2018-09-22T03:30:55Z

resource/resource.go

+
+// Detector attempts to detect resource information.
+// If the detector cannot find specific information, the respective Resource fields should
+// be left empty but no error should be returned.


This seems to imply its not valid to return nil, nil (saying that you should leave fields empty), though that seems like it would be the most natural way to return "this detector does not apply in the current environment".

resource/resource.go

Signed-off-by: Fabian Reinartz <[email protected]>

semistrict

Looks good. One thing I'm still not clear on is how this will be used. Is it going to be used to replace the existing resource package in the Stackdriver repo? Is there any way to show an example of its usage here?

resource/resource.go

Signed-off-by: Fabian Reinartz <[email protected]>

bogdandrutu · 2018-10-11T19:18:21Z

resource/resource.go

+	Labels map[string]string
+}
+
+// EncodeLabels encodes a labels map to a string as provided via the OC_RESOURCE_labelS environment variable.


OC_RESOURCE_labelS probably OC_RESOURCE_LABELS

bogdandrutu · 2018-10-11T19:18:38Z

resource/resource.go

+}
+
+// EncodeLabels encodes a labels map to a string as provided via the OC_RESOURCE_labelS environment variable.
+func EncodeLabels(labels map[string]string) string {


Do you need this to be public (exported)?

It's not used internally at all, so it's only here for public use. Seemed good to have and we already have a use case with the tool that runs discoveries and produces envvars.

You can document that most users will depend on FromEnv in the godoc.

In which one? Seems odd to document that getter X will be used more frequently in the docstring of a encoder.

Documenting what most users might prefer in godoc is a style we often use at Google and elsewhere. We don't have to do it, I just want users to have an easier time finding an entry point.

bogdandrutu · 2018-10-11T19:18:58Z

resource/resource.go

+// DecodeLabels decodes a serialized label map as used in the OC_RESOURCE_labelS variable.
+// A list of labels of the form `<key1>="<value1>",<key2>="<value2>",...` is accepted.
+// Domain names and paths are accepted as label keys.
+func DecodeLabels(s string) (map[string]string, error) {


Same comment about public.

Since we have EncodeLabels public, it seems to make sense to have the symmetric encoding function as well.

I think there's an argument to make both EncodeLabels and DecodeLabels private and just document the expected format in the FromEnv godoc: How do we expect users to use EncodeLabels? Won't they usually just set environment variables in something like a k8s yaml file or shell script? If so, how does it help to have a Go function exported?

We already have a use case for EncodeLabels here. I think it is a very legitimate one and the Encode/Decode symmetry should generally be given.

bogdandrutu · 2018-10-11T19:19:11Z

resource/resource.go

+
+var labelRegex = regexp.MustCompile(`^\s*([[:ascii:]]{1,256}?)=("[[:ascii:]]{0,256}?")\s*,`)
+
+// DecodeLabels decodes a serialized label map as used in the OC_RESOURCE_labelS variable.


Same comment about the env variable name.

bogdandrutu · 2018-10-11T19:20:01Z

resource/resource.go

+type Detector func(context.Context) (*Resource, error)
+
+// NewDetectorFromResource returns a detector that will always return resource r.
+func NewDetectorFromResource(r *Resource) Detector {


This is not necessary needed? as @rakyll always told me this is 1 line for users to write.

bogdandrutu · 2018-10-11T19:21:50Z

resource/resource.go

+var _ Detector = FromEnv
+
+// Merge resource information from b into a. In case of a collision, a takes precedence.
+func Merge(a, b *Resource) *Resource {


Is this important to be public? People can call the Chained version.

resource/resource.go

bogdandrutu · 2018-10-11T19:23:26Z

resource/resource.go

+// ChainedDetector returns a Detector that calls all input detectors sequentially an
+// merges each result with the previous one.
+// It returns on the first error that a sub-detector encounters.
+func ChainedDetector(detectors ...Detector) Detector {


Maybe FromDetectors? I will let @rakyll propose a good name for this.

What about MultiDetector a la io.MultiReader?

resource/resource.go

Signed-off-by: Fabian Reinartz <[email protected]>

rakyll · 2018-10-11T23:19:50Z

resource/resource.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// Package resource defines the resource type and provides helpers to derive them as well


Explain what a resource type is rather than saying resource package contains the type.

Signed-off-by: Fabian Reinartz <[email protected]>

fabxc · 2018-10-22T11:19:18Z

@rakyll @bogdandrutu @Ramonza could you take a final look after the recent changes?

semistrict · 2018-10-22T16:05:52Z

resource/resource.go

+// DecodeLabels decodes a serialized label map as used in the OC_RESOURCE_labelS variable.
+// A list of labels of the form `<key1>="<value1>",<key2>="<value2>",...` is accepted.
+// Domain names and paths are accepted as label keys.
+func DecodeLabels(s string) (map[string]string, error) {


I think there's an argument to make both EncodeLabels and DecodeLabels private and just document the expected format in the FromEnv godoc: How do we expect users to use EncodeLabels? Won't they usually just set environment variables in something like a k8s yaml file or shell script? If so, how does it help to have a Go function exported?

semistrict · 2018-10-22T16:10:32Z

resource/resource.go

+	}
+	labels := strings.TrimSpace(os.Getenv(EnvVarLabels))
+	if labels == "" {
+		return res, nil


If EnvVarType is empty, wouldn't it make more sense to return a nil resource here?

The type is not mandatory per our specs so I don't think we necessarily have to handle it specially here.
In general the empty resource is equivalent to the unset resource. Just that an empty struct requires less special handling by the caller, which seems preferable.

Add resource package

e71bc5a

This package adds resources as a core concept to OpenCensus. Signed-off-by: Fabian Reinartz <[email protected]>

fabxc force-pushed the resource branch from 24af6fa to e71bc5a Compare August 22, 2018 16:56

rakyll reviewed Aug 23, 2018

View reviewed changes

fabxc mentioned this pull request Aug 31, 2018

Add resource specs census-instrumentation/opencensus-specs#162

Merged

Address comments

4b3bba9

Signed-off-by: Fabian Reinartz <[email protected]>

rghetia reviewed Sep 12, 2018

View reviewed changes

Added and fixed tests

361940e

Signed-off-by: Fabian Reinartz <[email protected]>

Make Detectall unexported

2e51332

Signed-off-by: Fabian Reinartz <[email protected]>

rghetia mentioned this pull request Sep 17, 2018

pkg/monitoredresource.Autodetect(): causes me to vendor aws-sdk-go census-ecosystem/opencensus-go-exporter-stackdriver#35

Closed

semistrict approved these changes Sep 22, 2018

View reviewed changes

Address comments

06fb0e2

Signed-off-by: Fabian Reinartz <[email protected]>

semistrict approved these changes Sep 24, 2018

View reviewed changes

resource/resource.go Show resolved Hide resolved

Fabian Reinartz added 3 commits October 10, 2018 14:38

Rename Tags to Labels, expose envvar names

8bc50df

Signed-off-by: Fabian Reinartz <[email protected]>

Adjust label parsing to new syntax spec

41841a5

Signed-off-by: Fabian Reinartz <[email protected]>

Fix replace error

0222b25

Signed-off-by: Fabian Reinartz <[email protected]>

bogdandrutu reviewed Oct 11, 2018

View reviewed changes

rghetia reviewed Oct 11, 2018

View reviewed changes

resource/resource.go Show resolved Hide resolved

Address comments

90b9980

Signed-off-by: Fabian Reinartz <[email protected]>

rakyll reviewed Oct 11, 2018

View reviewed changes

Address comments

3b6d965

Signed-off-by: Fabian Reinartz <[email protected]>

semistrict approved these changes Oct 22, 2018

View reviewed changes

fabxc merged commit 4f7fcb4 into census-instrumentation:master Oct 29, 2018

odeke-em mentioned this pull request Nov 2, 2018

all: add a section describing the resource entity census-instrumentation/opencensus-website#416

Open


		var tagRegex = regexp.MustCompile(`\s([a-zA-Z0-9-_./]+)=(?:(".?")\|([^\s="]+))\s*,`)

		// DecodeTags decodes serialized a tag map as used in the OC_RESOURCE_TAGS variable.


		var labelRegex = regexp.MustCompile(`^\s([[:ascii:]]{1,256}?)=("[[:ascii:]]{0,256}?")\s,`)

		// DecodeLabels decodes a serialized label map as used in the OC_RESOURCE_labelS variable.

Add resource package #878

Add resource package #878

Conversation

fabxc commented Aug 22, 2018 • edited Loading

semistrict commented Aug 22, 2018

rakyll left a 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

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

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

fabxc commented Sep 13, 2018

rghetia commented Sep 13, 2018

rakyll commented Sep 22, 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

fabxc Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

semistrict left a 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

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

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

fabxc commented Oct 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabxc commented Aug 22, 2018 •

edited

Loading

fabxc Sep 24, 2018 •

edited

Loading