From fa0ad27c6f6f955caf1b3082c7fe8536f20eb0f5 Mon Sep 17 00:00:00 2001 From: Pablo Baeyens Date: Thu, 11 Jul 2024 17:57:25 +0200 Subject: [PATCH 1/4] [extension/observer/docker, receiver/dockerstats] Add hint about api_version --- extension/observer/dockerobserver/config.go | 16 ++++++++++++++++ receiver/dockerstatsreceiver/config.go | 17 +++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/extension/observer/dockerobserver/config.go b/extension/observer/dockerobserver/config.go index 743a7cd9025a..e0de00a8ecbb 100644 --- a/extension/observer/dockerobserver/config.go +++ b/extension/observer/dockerobserver/config.go @@ -9,6 +9,7 @@ import ( "time" "github.com/open-telemetry/opentelemetry-collector-contrib/internal/docker" + "go.opentelemetry.io/collector/confmap" ) // Config defines configuration for docker observer @@ -64,3 +65,18 @@ func (config Config) Validate() error { } return nil } + +func (config *Config) Unmarshal(conf *confmap.Conf) error { + err := conf.Unmarshal(config) + if err != nil { + if floatAPIVersion, ok := conf.Get("api_version").(float64); ok { + return fmt.Errorf( + "%w.\n\nHint: You may want to wrap the 'api_version' value in quotes (api_version: \"%1.2f\")", + err, + floatAPIVersion, + ) + } + return err + } + return nil +} diff --git a/receiver/dockerstatsreceiver/config.go b/receiver/dockerstatsreceiver/config.go index 86825de8c65e..96085e513a9f 100644 --- a/receiver/dockerstatsreceiver/config.go +++ b/receiver/dockerstatsreceiver/config.go @@ -5,8 +5,10 @@ package dockerstatsreceiver // import "github.com/open-telemetry/opentelemetry-c import ( "errors" + "fmt" "go.opentelemetry.io/collector/component" + "go.opentelemetry.io/collector/confmap" "go.opentelemetry.io/collector/receiver/scraperhelper" "github.com/open-telemetry/opentelemetry-collector-contrib/internal/docker" @@ -54,3 +56,18 @@ func (config Config) Validate() error { } return nil } + +func (config *Config) Unmarshal(conf *confmap.Conf) error { + err := conf.Unmarshal(config) + if err != nil { + if floatAPIVersion, ok := conf.Get("api_version").(float64); ok { + return fmt.Errorf( + "%w.\n\nHint: You may want to wrap the 'api_version' value in quotes (api_version: \"%1.2f\")", + err, + floatAPIVersion, + ) + } + return err + } + return nil +} From 32eaea32ea7d86609bccc669e9cce037decf47fd Mon Sep 17 00:00:00 2001 From: Pablo Baeyens Date: Thu, 11 Jul 2024 18:40:59 +0200 Subject: [PATCH 2/4] Fix formatting --- extension/observer/dockerobserver/config.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/extension/observer/dockerobserver/config.go b/extension/observer/dockerobserver/config.go index e0de00a8ecbb..4e3bb293beae 100644 --- a/extension/observer/dockerobserver/config.go +++ b/extension/observer/dockerobserver/config.go @@ -8,8 +8,9 @@ import ( "fmt" "time" - "github.com/open-telemetry/opentelemetry-collector-contrib/internal/docker" "go.opentelemetry.io/collector/confmap" + + "github.com/open-telemetry/opentelemetry-collector-contrib/internal/docker" ) // Config defines configuration for docker observer From 8e59e82383caaa17221db678749ddd0e439da913 Mon Sep 17 00:00:00 2001 From: Pablo Baeyens Date: Fri, 12 Jul 2024 17:37:02 +0200 Subject: [PATCH 3/4] Changelog entries --- ...i_api-version-explicit-error-observer.yaml | 27 +++++++++++++++++++ .../mx-psi_api-version-explicit-error.yaml | 27 +++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 .chloggen/mx-psi_api-version-explicit-error-observer.yaml create mode 100644 .chloggen/mx-psi_api-version-explicit-error.yaml diff --git a/.chloggen/mx-psi_api-version-explicit-error-observer.yaml b/.chloggen/mx-psi_api-version-explicit-error-observer.yaml new file mode 100644 index 000000000000..798b2ed869d4 --- /dev/null +++ b/.chloggen/mx-psi_api-version-explicit-error-observer.yaml @@ -0,0 +1,27 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) +component: dockerobserver + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Add hint to error when using float for `api_version` field + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [34043] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: + +# If your change doesn't affect end users or the exported elements of any package, +# you should instead start your pull request title with [chore] or use the "Skip Changelog" label. +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [] diff --git a/.chloggen/mx-psi_api-version-explicit-error.yaml b/.chloggen/mx-psi_api-version-explicit-error.yaml new file mode 100644 index 000000000000..ac05a8cae544 --- /dev/null +++ b/.chloggen/mx-psi_api-version-explicit-error.yaml @@ -0,0 +1,27 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) +component: dockerstatsreceiver + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Add hint to error when using float for `api_version` field + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [34043] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: + +# If your change doesn't affect end users or the exported elements of any package, +# you should instead start your pull request title with [chore] or use the "Skip Changelog" label. +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [] From 82da78b8d53b5223cf990e3da12fadf560eeb686 Mon Sep 17 00:00:00 2001 From: Pablo Baeyens Date: Mon, 15 Jul 2024 12:19:59 +0200 Subject: [PATCH 4/4] Add tests --- .../observer/dockerobserver/config_test.go | 30 +++++++++++++++--- .../testdata/api_version_float.yaml | 2 ++ .../testdata/api_version_string.yaml | 2 ++ receiver/dockerstatsreceiver/config_test.go | 31 +++++++++++++++---- .../testdata/api_version_float.yaml | 2 ++ .../testdata/api_version_string.yaml | 2 ++ 6 files changed, 58 insertions(+), 11 deletions(-) create mode 100644 extension/observer/dockerobserver/testdata/api_version_float.yaml create mode 100644 extension/observer/dockerobserver/testdata/api_version_string.yaml create mode 100644 receiver/dockerstatsreceiver/testdata/api_version_float.yaml create mode 100644 receiver/dockerstatsreceiver/testdata/api_version_string.yaml diff --git a/extension/observer/dockerobserver/config_test.go b/extension/observer/dockerobserver/config_test.go index 768cf8aa1a47..cab7d3342ede 100644 --- a/extension/observer/dockerobserver/config_test.go +++ b/extension/observer/dockerobserver/config_test.go @@ -11,6 +11,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.opentelemetry.io/collector/component" + "go.opentelemetry.io/collector/confmap" "go.opentelemetry.io/collector/confmap/confmaptest" "github.com/open-telemetry/opentelemetry-collector-contrib/extension/observer/dockerobserver/internal/metadata" @@ -74,14 +75,33 @@ func TestValidateConfig(t *testing.T) { assert.Nil(t, component.ValidateConfig(cfg)) } -func loadConfig(t testing.TB, id component.ID) *Config { - cm, err := confmaptest.LoadConf(filepath.Join("testdata", "config.yaml")) +func loadConf(t testing.TB, path string, id component.ID) *confmap.Conf { + cm, err := confmaptest.LoadConf(filepath.Join("testdata", path)) require.NoError(t, err) - factory := NewFactory() - cfg := factory.CreateDefaultConfig() sub, err := cm.Sub(id.String()) require.NoError(t, err) - require.NoError(t, sub.Unmarshal(cfg)) + return sub +} +func loadConfig(t testing.TB, id component.ID) *Config { + factory := NewFactory() + cfg := factory.CreateDefaultConfig() + sub := loadConf(t, "config.yaml", id) + require.NoError(t, sub.Unmarshal(cfg)) return cfg.(*Config) } + +func TestApiVersionCustomError(t *testing.T) { + sub := loadConf(t, "api_version_float.yaml", component.NewID(metadata.Type)) + factory := NewFactory() + cfg := factory.CreateDefaultConfig() + err := sub.Unmarshal(cfg) + require.Error(t, err) + assert.Contains(t, err.Error(), + `Hint: You may want to wrap the 'api_version' value in quotes (api_version: "1.40")`, + ) + + sub = loadConf(t, "api_version_string.yaml", component.NewID(metadata.Type)) + err = sub.Unmarshal(cfg) + require.NoError(t, err) +} diff --git a/extension/observer/dockerobserver/testdata/api_version_float.yaml b/extension/observer/dockerobserver/testdata/api_version_float.yaml new file mode 100644 index 000000000000..18f512e32e41 --- /dev/null +++ b/extension/observer/dockerobserver/testdata/api_version_float.yaml @@ -0,0 +1,2 @@ +docker_observer: + api_version: 1.40 diff --git a/extension/observer/dockerobserver/testdata/api_version_string.yaml b/extension/observer/dockerobserver/testdata/api_version_string.yaml new file mode 100644 index 000000000000..7f83b7ba30f7 --- /dev/null +++ b/extension/observer/dockerobserver/testdata/api_version_string.yaml @@ -0,0 +1,2 @@ +docker_observer: + api_version: "1.40" diff --git a/receiver/dockerstatsreceiver/config_test.go b/receiver/dockerstatsreceiver/config_test.go index 490f0d2f2eb7..4ce59bfea1b7 100644 --- a/receiver/dockerstatsreceiver/config_test.go +++ b/receiver/dockerstatsreceiver/config_test.go @@ -13,12 +13,21 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.opentelemetry.io/collector/component" + "go.opentelemetry.io/collector/confmap" "go.opentelemetry.io/collector/confmap/confmaptest" "go.opentelemetry.io/collector/receiver/scraperhelper" "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/dockerstatsreceiver/internal/metadata" ) +func loadConf(t testing.TB, path string, id component.ID) *confmap.Conf { + cm, err := confmaptest.LoadConf(filepath.Join("testdata", path)) + require.NoError(t, err) + sub, err := cm.Sub(id.String()) + require.NoError(t, err) + return sub +} + func TestLoadConfig(t *testing.T) { t.Parallel() @@ -72,14 +81,9 @@ func TestLoadConfig(t *testing.T) { for _, tt := range tests { t.Run(tt.id.String(), func(t *testing.T) { - cm, err := confmaptest.LoadConf(filepath.Join("testdata", "config.yaml")) - require.NoError(t, err) - + sub := loadConf(t, "config.yaml", tt.id) factory := NewFactory() cfg := factory.CreateDefaultConfig() - - sub, err := cm.Sub(tt.id.String()) - require.NoError(t, err) require.NoError(t, sub.Unmarshal(cfg)) assert.NoError(t, component.ValidateConfig(cfg)) @@ -108,3 +112,18 @@ func TestValidateErrors(t *testing.T) { } assert.Equal(t, `"collection_interval": requires positive value`, component.ValidateConfig(cfg).Error()) } + +func TestApiVersionCustomError(t *testing.T) { + sub := loadConf(t, "api_version_float.yaml", component.NewID(metadata.Type)) + factory := NewFactory() + cfg := factory.CreateDefaultConfig() + err := sub.Unmarshal(cfg) + require.Error(t, err) + assert.Contains(t, err.Error(), + `Hint: You may want to wrap the 'api_version' value in quotes (api_version: "1.40")`, + ) + + sub = loadConf(t, "api_version_string.yaml", component.NewID(metadata.Type)) + err = sub.Unmarshal(cfg) + require.NoError(t, err) +} diff --git a/receiver/dockerstatsreceiver/testdata/api_version_float.yaml b/receiver/dockerstatsreceiver/testdata/api_version_float.yaml new file mode 100644 index 000000000000..3c39b2aaaa89 --- /dev/null +++ b/receiver/dockerstatsreceiver/testdata/api_version_float.yaml @@ -0,0 +1,2 @@ +docker_stats: + api_version: 1.40 diff --git a/receiver/dockerstatsreceiver/testdata/api_version_string.yaml b/receiver/dockerstatsreceiver/testdata/api_version_string.yaml new file mode 100644 index 000000000000..f34ecb350e90 --- /dev/null +++ b/receiver/dockerstatsreceiver/testdata/api_version_string.yaml @@ -0,0 +1,2 @@ +docker_stats: + api_version: "1.40"