From 19690d13abb220829a9f111d0c9d5977360c4783 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Sun, 19 Jun 2022 20:32:01 -0400 Subject: [PATCH 1/5] Decode values from OTEL_RESOURCE_ATTRIBUTES The W3C spec specifies that values must be percent-encoded so when reading the environment variable `OTEL_RESOURCE_ATTRIBUTES` the SDK should decode them. This is done by the `baggage` package, but its behaviour in case of errors is slightly different from the current implementation of the SDK, more specifically in cases where a key is missing a value. The SDK returns a partial resource while the `bagage` package returns nil. This may be considered a breaking change, so this commit fixes the current implementation instead of using `baggage.Parse`. --- sdk/resource/env.go | 8 +++++++- sdk/resource/env_test.go | 7 ++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/sdk/resource/env.go b/sdk/resource/env.go index 531a68424a0..18a0ac8cbb8 100644 --- a/sdk/resource/env.go +++ b/sdk/resource/env.go @@ -17,6 +17,7 @@ package resource // import "go.opentelemetry.io/otel/sdk/resource" import ( "context" "fmt" + "net/url" "os" "strings" @@ -88,7 +89,12 @@ func constructOTResources(s string) (*Resource, error) { invalid = append(invalid, p) continue } - k, v := strings.TrimSpace(field[0]), strings.TrimSpace(field[1]) + k := strings.TrimSpace(field[0]) + v, err := url.QueryUnescape(strings.TrimSpace(field[1])) + if err != nil { + // Retain original value if decoding fails. + v = field[1] + } attrs = append(attrs, attribute.String(k, v)) } var err error diff --git a/sdk/resource/env_test.go b/sdk/resource/env_test.go index 8d1952e5f62..6c4dc5053c2 100644 --- a/sdk/resource/env_test.go +++ b/sdk/resource/env_test.go @@ -43,7 +43,7 @@ func TestDetectOnePair(t *testing.T) { func TestDetectMultiPairs(t *testing.T) { store, err := ottest.SetEnvVariables(map[string]string{ "x": "1", - resourceAttrKey: "key=value, k = v , a= x, a=z", + resourceAttrKey: "key=value, k = v , a= x, a=z, b=c%2Fd", }) require.NoError(t, err) defer func() { require.NoError(t, store.Restore()) }() @@ -51,12 +51,13 @@ func TestDetectMultiPairs(t *testing.T) { detector := &fromEnv{} res, err := detector.Detect(context.Background()) require.NoError(t, err) - assert.Equal(t, res, NewSchemaless( + assert.Equal(t, NewSchemaless( attribute.String("key", "value"), attribute.String("k", "v"), attribute.String("a", "x"), attribute.String("a", "z"), - )) + attribute.String("b", "c/d"), + ), res) } func TestEmpty(t *testing.T) { From 5f003fd49a8ed089a088d64af745f77501b938b5 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Sun, 19 Jun 2022 21:05:25 -0400 Subject: [PATCH 2/5] Add changelog entry for #2963 --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca0334fbcea..5dfe738b5dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Support for go1.16. Support is now only for go1.17 and go1.18 (#2917) +### Fixed + +- Decode urlencoded values from the `OTEL_RESOURCE_ATTRIBUTES` environment variable (#2963) + ## [1.7.0/0.30.0] - 2022-04-28 ### Added From 234c3095aaf217bc07d9a6a0d6c4309d227e3583 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Wed, 22 Jun 2022 18:25:52 -0400 Subject: [PATCH 3/5] Use otel.Handle on OTEL_RESOURCE_ATTRIBUTES decode error --- sdk/resource/env.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/resource/env.go b/sdk/resource/env.go index 18a0ac8cbb8..982da54c563 100644 --- a/sdk/resource/env.go +++ b/sdk/resource/env.go @@ -21,6 +21,7 @@ import ( "os" "strings" + "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" semconv "go.opentelemetry.io/otel/semconv/v1.10.0" ) @@ -92,8 +93,7 @@ func constructOTResources(s string) (*Resource, error) { k := strings.TrimSpace(field[0]) v, err := url.QueryUnescape(strings.TrimSpace(field[1])) if err != nil { - // Retain original value if decoding fails. - v = field[1] + otel.Handle(err) } attrs = append(attrs, attribute.String(k, v)) } From e7faa0906af2bb080beb81cef331f32792d84782 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Thu, 23 Jun 2022 11:18:05 -0400 Subject: [PATCH 4/5] retain original value when decoding fails --- sdk/resource/env.go | 3 +++ sdk/resource/env_test.go | 15 +++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/sdk/resource/env.go b/sdk/resource/env.go index 982da54c563..9ab93567aaf 100644 --- a/sdk/resource/env.go +++ b/sdk/resource/env.go @@ -93,6 +93,9 @@ func constructOTResources(s string) (*Resource, error) { k := strings.TrimSpace(field[0]) v, err := url.QueryUnescape(strings.TrimSpace(field[1])) if err != nil { + // Retain original value if decoding fails, otherwise it will be + // an empty string. + v = field[1] otel.Handle(err) } attrs = append(attrs, attribute.String(k, v)) diff --git a/sdk/resource/env_test.go b/sdk/resource/env_test.go index 6c4dc5053c2..7334ee28a43 100644 --- a/sdk/resource/env_test.go +++ b/sdk/resource/env_test.go @@ -103,6 +103,21 @@ func TestMissingKeyError(t *testing.T) { )) } +func TestInvalidPercentDecoding(t *testing.T) { + store, err := ottest.SetEnvVariables(map[string]string{ + resourceAttrKey: "key=%invalid", + }) + require.NoError(t, err) + defer func() { require.NoError(t, store.Restore()) }() + + detector := &fromEnv{} + res, err := detector.Detect(context.Background()) + assert.NoError(t, err) + assert.Equal(t, NewSchemaless( + attribute.String("key", "%invalid"), + ), res) +} + func TestDetectServiceNameFromEnv(t *testing.T) { store, err := ottest.SetEnvVariables(map[string]string{ resourceAttrKey: "key=value,service.name=foo", From 8af74a279b4585670811850e16b66a973ff098c6 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Wed, 19 Oct 2022 12:04:11 -0400 Subject: [PATCH 5/5] docs: update CHANGELOG --- CHANGELOG.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 37658fc5eac..e95db77ae63 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Changed +- Decode urlencoded values from the `OTEL_RESOURCE_ATTRIBUTES` environment variable. (#2963) - `sdktrace.TraceProvider.Shutdown` and `sdktrace.TraceProvider.ForceFlush` to not return error when no processor register. (#3268) - The `"go.opentelemetry.io/otel/exporters/prometheus".New` now also returns an error indicating the failure to register the exporter with Prometheus. (#3239) - The prometheus exporter will no longer try to enumerate the metrics it will send to prometheus on startup. @@ -130,10 +131,6 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - All exporters will be shutdown even if one reports an error (#3091) - Ensure valid UTF-8 when truncating over-length attribute values. (#3156) -### Changed - -- Decode urlencoded values from the `OTEL_RESOURCE_ATTRIBUTES` environment variable. (#2963) - ## [1.9.0/0.0.3] - 2022-08-01 ### Added