Add ability to set Global tags using code and environment variables #30

ccaraman · 2020-04-20T23:49:09Z

Adding functionality to set tags for all spans using code and/or environment variables
Updated the readme to follow the java documentation structure for detailing various ways to configure the instrumentation.
Added a check in the TestEmptyContext for operation name to be session.

ccaraman · 2020-04-21T00:22:44Z

@brianashby-sfx - I updated the readme if you want to take a look.

brianashby · 2020-04-21T00:40:05Z

README.md

-`SIGNALFX_ACCESS_TOKEN` / [WithAccessToken](https://godoc.org/github.com/signalfx/signalfx-go-tracing/tracing/#WithAccessToken) (no default)
+| Code | Environment Variable | Default Value | Notes |
+| ---  | ---                  | ---           | ---   |
+| [WithServiceName](https://godoc.org/github.com/signalfx/signalfx-go-tracing/tracing/#WithServiceName) | `SIGNALFX_SERVICE_NAME` | `SignalFx-Tracing` | Name identifying the service as a whole |


Can we use this for the description:

The name of the service.

pjanotti

I'm ignoring all circle CI stuff :) but a related Q: since there will be a go.mod for CI isn't better to add it to the project as a whole? Are trying to avoid changes that will conflict with upstream?

tracing/tracing.go

pjanotti · 2020-04-21T00:28:23Z

tracing/tracing.go

@@ -65,16 +94,30 @@ func WithEndpointURL(url string) StartOption {
 	}
 }

+// WithGlobalTag sets a key/value pair which will be set as a tag on all spans


Suggested change

// WithGlobalTag sets a key/value pair which will be set as a tag on all spans

// WithGlobalTag sets a tag with the given key/value on all spans

pjanotti · 2020-04-21T00:29:34Z

tracing/tracing.go

+// key1:value1,
+func envGlobalTags() []tracer.StartOption {
+	var globalTags []tracer.StartOption
+	if val := os.Getenv(signalfxSpanTags); val != "" {


nit: in golang is more idiomatic to check for val == "" empty and bail instead of nesting the for inside the if.

tracing/tracing.go

pjanotti · 2020-04-21T00:37:08Z

tracing/tracing.go

+			if len(pair) == 2 {
+				key := strings.TrimSpace(pair[0])
+				value := strings.TrimSpace(pair[1])
+				if key != "" && value != "" {


Is it really necessary to enforce non-empty values?

Yes - in testing if the value was empty it would add an entry to the map with the key and the empty value.

yes, but having a key with an empty value is different from the key not being present...

Doesn't it just seem like bad practice to have an empty value?

Some tracers will remove existing tags if they are set w/ an empty value. We definitely shouldn't be using them in zipkin encoding at the least, so this is a good todo.

This is not OTel but fwiw the OTel spec says: "Attribute values expressing a numerical value of zero or an empty string are considered meaningful and MUST be stored and passed on to span processors/exporters. Attribute values of null are considered to be not set and get discarded as if that SetAttribute call had never been made."

Hmm good to know about the spec.
@rmfitzpatrick - do you know which tracers remove tags that are set with empty values?

oh - so i found this in the zipkin api https://github.com/openzipkin/zipkin-api/blob/master/zipkin-jsonv2.proto#L50-L51
it says that empty values are valid but empty keys are not. Since this is only for the SFx tracer, i'm going to remove the check for value being empty.

brianashby · 2020-04-21T00:41:19Z

README.md

+| Code | Environment Variable | Default Value | Notes |
+| ---  | ---                  | ---           | ---   |
+| [WithServiceName](https://godoc.org/github.com/signalfx/signalfx-go-tracing/tracing/#WithServiceName) | `SIGNALFX_SERVICE_NAME` | `SignalFx-Tracing` | Name identifying the service as a whole |
+| [WithEndpointURL](https://godoc.org/github.com/signalfx/signalfx-go-tracing/tracing/#WithEndpointURL) | `SIGNALFX_ENDPOINT_URL` | `http://localhost:9080/v1/trace` | URL to send traces to  |


To confirm, can this be a Smart Agent, SignalFx endpoint, or OpenTelemetry Collector? If so, let's specify here.

Suggestion for existing text:

The URL to send traces to.

Yes, it can send it to all of those backends.

brianashby · 2020-04-21T00:42:23Z

README.md

+| [WithServiceName](https://godoc.org/github.com/signalfx/signalfx-go-tracing/tracing/#WithServiceName) | `SIGNALFX_SERVICE_NAME` | `SignalFx-Tracing` | Name identifying the service as a whole |
+| [WithEndpointURL](https://godoc.org/github.com/signalfx/signalfx-go-tracing/tracing/#WithEndpointURL) | `SIGNALFX_ENDPOINT_URL` | `http://localhost:9080/v1/trace` | URL to send traces to  |
+| [WithAccessToken](https://godoc.org/github.com/signalfx/signalfx-go-tracing/tracing/#WithAccessToken) | `SIGNALFX_ACCESS_TOKEN` | none | |
+| [WithGlobalTag](https://godoc.org/github.com/signalfx/signalfx-go-tracing/tracing/#WithGlobalTag) | `SIGNALFX_SPAN_TAGS` | none | If specifying as an environment variable, the format is `key:value` with multiple values separated by commas. Ex.`"a:b, c:d"`|


Suggestion for description:

Comma-separated list of tags included in every reported span. For example, "key1:val1,key2:val2".

brianashby · 2020-04-21T00:43:55Z

README.md

+| [WithAccessToken](https://godoc.org/github.com/signalfx/signalfx-go-tracing/tracing/#WithAccessToken) | `SIGNALFX_ACCESS_TOKEN` | none | |
+| [WithGlobalTag](https://godoc.org/github.com/signalfx/signalfx-go-tracing/tracing/#WithGlobalTag) | `SIGNALFX_SPAN_TAGS` | none | If specifying as an environment variable, the format is `key:value` with multiple values separated by commas. Ex.`"a:b, c:d"`|
+
+Note: The current transport format is Zipkin which only supports string values for tags.


If this is for SIGNALFX_SPAN_TAGS, let's move it to the note for that config value.

ccaraman · 2020-04-21T00:48:03Z

I'm ignoring all circle CI stuff :) but a related Q: since there will be a go.mod for CI isn't better to add it to the project as a whole? Are trying to avoid changes that will conflict with upstream?

@pjanotti I pulled in @owais changes to make the tests pass. I'll sync with him tomorrow to see if he has any strong preferences.

pjanotti · 2020-04-21T00:58:24Z

@ccaraman sounds good regarding CircleCI

rmfitzpatrick · 2020-04-21T17:43:23Z

@ccaraman would you mind rebasing once #29 lands?

rmfitzpatrick · 2020-04-21T17:58:18Z

tracing/tracing.go

+// envGlobalTags extract global tags from the environment variable and parses the value in the expected format
+// key1:value1,
+func envGlobalTags() []tracer.StartOption {
+	var globalTags []tracer.StartOption


This would be a good opportunity to tag the library and version by default:
signalfx.tracing.library: "go-tracing" *
signalfx.tracing.version: someNewConstantWeUpdateWithEachGitTag.

I'll do that in a separate PR.

owais · 2020-04-21T19:38:51Z

I'm ignoring all circle CI stuff :) but a related Q: since there will be a go.mod for CI isn't better to add it to the project as a whole? Are trying to avoid changes that will conflict with upstream?

We don't want to add the libraries we instrument as dependencies so other projects that use this library don't need to pull in everything we support. At the same time we need all the dependencies to run unit and integration tests so we keep the go.mod/sum files in .circleci/ directory and copy them to root on CI before running the tests.

I think with Go modules, each instrumentation could be an independent Go module. We still wouldn't add the libraries we instrument as deps but it would allow adding other 3rd party libs. I'll think more about it. We should move to Go modules anyway at some point.

owais

Any reason to prefer <key>:<value>: over <key>=<value>? I think : is more likely to be in a string than =. It is often used to separate namespaces and values for example.

Also OpenCensus used the equal to sign for OC_RESOURCE_LABELS which was a very similar feature.

ccaraman · 2020-04-21T22:56:08Z

Any reason to prefer <key>:<value>: over <key>=<value>? I think : is more likely to be in a string than =. It is often used to separate namespaces and values for example.

Also OpenCensus used the equal to sign for OC_RESOURCE_LABELS which was a very similar feature.

This is what is used in other SFx libraries, but I didn't know about Oc having this feature as well. Let me start a thread offline.

owais · 2020-04-21T22:59:08Z

👍 ref: https://godoc.org/go.opencensus.io/resource#DecodeLabels

.circleci/config.yml

.circleci/go.mod

rmfitzpatrick

💣

ccaraman added 3 commits April 20, 2020 16:46

Add ability to set Global tags using code and environment variables

23259d3

Pull in Owais' circle ci changes

c23a7ee

Forgot a file

880e66d

ccaraman requested review from a user and rmfitzpatrick and removed request for a user April 21, 2020 00:10

brianashby reviewed Apr 21, 2020

View reviewed changes

pjanotti reviewed Apr 21, 2020

View reviewed changes

brianashby reviewed Apr 21, 2020

View reviewed changes

rmfitzpatrick reviewed Apr 21, 2020

View reviewed changes

ccaraman added 4 commits April 21, 2020 13:36

Address PR comments

9d614bd

Merge branch 'master' into add_global_span_tags

1c4da66

Missed merge remnant

618a9d9

Out of practice merging code bases

6d1efa0

owais reviewed Apr 21, 2020

View reviewed changes

pjanotti previously approved these changes Apr 22, 2020

View reviewed changes

rmfitzpatrick reviewed Apr 22, 2020

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

rmfitzpatrick reviewed Apr 22, 2020

View reviewed changes

.circleci/go.mod Outdated Show resolved Hide resolved

It wasn't just a nit

8f7bdc4

ccaraman dismissed pjanotti’s stale review via 8f7bdc4 April 22, 2020 17:52

rmfitzpatrick approved these changes Apr 22, 2020

View reviewed changes

pjanotti approved these changes Apr 22, 2020

View reviewed changes

ccaraman merged commit c5cdba9 into master Apr 22, 2020

pellared deleted the add_global_span_tags branch March 25, 2021 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to set Global tags using code and environment variables #30

Add ability to set Global tags using code and environment variables #30

ccaraman commented Apr 20, 2020

ccaraman commented Apr 21, 2020

brianashby Apr 21, 2020

pjanotti left a comment

pjanotti Apr 21, 2020

pjanotti Apr 21, 2020

pjanotti Apr 21, 2020

ccaraman Apr 21, 2020

pjanotti Apr 21, 2020

ccaraman Apr 21, 2020

rmfitzpatrick Apr 21, 2020

pjanotti Apr 21, 2020

ccaraman Apr 21, 2020

ccaraman Apr 21, 2020

brianashby Apr 21, 2020

ccaraman Apr 21, 2020

brianashby Apr 21, 2020

brianashby Apr 21, 2020

ccaraman commented Apr 21, 2020 •

edited

Loading

pjanotti commented Apr 21, 2020

rmfitzpatrick commented Apr 21, 2020

rmfitzpatrick Apr 21, 2020 •

edited

Loading

ccaraman Apr 21, 2020

owais commented Apr 21, 2020 •

edited

Loading

owais left a comment

ccaraman commented Apr 21, 2020

owais commented Apr 21, 2020

rmfitzpatrick left a comment

	// WithGlobalTag sets a key/value pair which will be set as a tag on all spans
	// WithGlobalTag sets a tag with the given key/value on all spans

Add ability to set Global tags using code and environment variables #30

Add ability to set Global tags using code and environment variables #30

Conversation

ccaraman commented Apr 20, 2020

ccaraman commented Apr 21, 2020

Choose a reason for hiding this comment

pjanotti 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

ccaraman commented Apr 21, 2020 • edited Loading

pjanotti commented Apr 21, 2020

rmfitzpatrick commented Apr 21, 2020

rmfitzpatrick Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owais commented Apr 21, 2020 • edited Loading

owais left a comment

Choose a reason for hiding this comment

ccaraman commented Apr 21, 2020

owais commented Apr 21, 2020

rmfitzpatrick left a comment

Choose a reason for hiding this comment

ccaraman commented Apr 21, 2020 •

edited

Loading

rmfitzpatrick Apr 21, 2020 •

edited

Loading

owais commented Apr 21, 2020 •

edited

Loading