Skip to content

Commit

Permalink
[otelcol] Add hint about confmap.strictlyTypedInput feature gate (#…
Browse files Browse the repository at this point in the history
…10553)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

<!-- Issue number if applicable -->

Adds coda to errors related to `confmap.strictlyTypedInput` that direct
users to #10552 and explain the feature gate.

#### Link to tracking issue

Updates #9532

<!--Describe what testing was performed and which tests were added.-->
#### Testing

Tests that the coda is present when there are weakly typed errors and
not present with non weakly-typed errors.

<!--Describe the documentation added.-->
  • Loading branch information
mx-psi authored Jul 9, 2024
1 parent 9d990b9 commit e7ce1d5
Show file tree
Hide file tree
Showing 12 changed files with 436 additions and 1 deletion.
25 changes: 25 additions & 0 deletions .chloggen/mx-psi_clearer-error-message.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# 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. otlpreceiver)
component: confmap

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Add explanation to errors related to `confmap.strictlyTypedInput` feature gate.

# One or more tracking issues or pull requests related to the change
issues: [9532]

# (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:

# 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: []
29 changes: 28 additions & 1 deletion otelcol/configprovider.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,16 @@ package otelcol // import "go.opentelemetry.io/collector/otelcol"
import (
"context"
"fmt"
"strings"

"go.opentelemetry.io/collector/confmap"
"go.opentelemetry.io/collector/internal/featuregates"
)

var (
strictlyTypedMessageCoda = `Hint: Temporarily restore the previous behavior by disabling
the ` + fmt.Sprintf("`%s`", featuregates.StrictlyTypedInputID) + ` feature gate. More details at:
https://github.com/open-telemetry/opentelemetry-collector/issues/10552`
)

// ConfigProvider provides the service configuration.
Expand Down Expand Up @@ -95,7 +103,26 @@ func (cm *configProvider) Get(ctx context.Context, factories Factories) (*Config

var cfg *configSettings
if cfg, err = unmarshal(conf, factories); err != nil {
return nil, fmt.Errorf("cannot unmarshal the configuration: %w", err)
err = fmt.Errorf("cannot unmarshal the configuration: %w", err)

if featuregates.StrictlyTypedInputGate.IsEnabled() {
var shouldAddCoda bool
for _, errorStr := range []string{
"got unconvertible type", // https://github.com/mitchellh/mapstructure/blob/8508981/mapstructure.go#L610
"source data must be", // https://github.com/mitchellh/mapstructure/blob/8508981/mapstructure.go#L1114
"expected a map, got 'slice'", // https://github.com/mitchellh/mapstructure/blob/8508981/mapstructure.go#L831
} {
shouldAddCoda = strings.Contains(err.Error(), errorStr)
if shouldAddCoda {
break
}
}
if shouldAddCoda {
err = fmt.Errorf("%w\n\n%s", err, strictlyTypedMessageCoda)
}
}

return nil, err
}

return &Config{
Expand Down
74 changes: 74 additions & 0 deletions otelcol/configprovider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import (
"gopkg.in/yaml.v3"

"go.opentelemetry.io/collector/confmap"
"go.opentelemetry.io/collector/featuregate"
"go.opentelemetry.io/collector/internal/featuregates"
)

func newConfig(yamlBytes []byte, factories Factories) (*Config, error) {
Expand Down Expand Up @@ -137,3 +139,75 @@ func TestGetConfmap(t *testing.T) {

assert.EqualValues(t, yamlMap, cmap.ToStringMap())
}

func TestStrictlyTypedCoda(t *testing.T) {
tests := []struct {
basename string
// isErrFromStrictTypes indicates whether the test should expect an error when the feature gate is
// disabled. If so, we check that it errs both with and without the feature gate and that the coda is never
// present.
isErrFromStrictTypes bool
}{
{basename: "weak-implicit-bool-to-string.yaml"},
{basename: "weak-implicit-int-to-string.yaml"},
{basename: "weak-implicit-bool-to-int.yaml"},
{basename: "weak-implicit-string-to-int.yaml"},
{basename: "weak-implicit-int-to-bool.yaml"},
{basename: "weak-implicit-string-to-bool.yaml"},
{basename: "weak-empty-map-to-empty-array.yaml"},
{basename: "weak-slice-of-maps-to-map.yaml"},
{basename: "weak-single-element-to-slice.yaml"},
{
basename: "otelcol-invalid-components.yaml",
isErrFromStrictTypes: true,
},
}

for _, tt := range tests {
t.Run(tt.basename, func(t *testing.T) {
filename := filepath.Join("testdata", tt.basename)
uriLocation := "file:" + filename
fileProvider := newFakeProvider("file", func(_ context.Context, _ string, _ confmap.WatcherFunc) (*confmap.Retrieved, error) {
return confmap.NewRetrieved(newConfFromFile(t, filename))
})
cp, err := NewConfigProvider(ConfigProviderSettings{
ResolverSettings: confmap.ResolverSettings{
URIs: []string{uriLocation},
ProviderFactories: []confmap.ProviderFactory{fileProvider},
},
})
require.NoError(t, err)
factories, err := nopFactories()
require.NoError(t, err)

// Save the previous value of the feature gate and restore it after the test.
prev := featuregates.StrictlyTypedInputGate.IsEnabled()
defer func() {
require.NoError(t, featuregate.GlobalRegistry().Set(featuregates.StrictlyTypedInputID, prev))
}()

// Ensure the error does not appear with the feature gate disabled.
require.NoError(t, featuregate.GlobalRegistry().Set(featuregates.StrictlyTypedInputID, false))
_, errWeakTypes := cp.Get(context.Background(), factories)
if tt.isErrFromStrictTypes {
require.Error(t, errWeakTypes)
// Ensure coda is **NOT** present.
assert.NotContains(t, errWeakTypes.Error(), strictlyTypedMessageCoda)
} else {
require.NoError(t, errWeakTypes)
}

// Test with the feature gate enabled.
require.NoError(t, featuregate.GlobalRegistry().Set(featuregates.StrictlyTypedInputID, true))
_, errStrictTypes := cp.Get(context.Background(), factories)
require.Error(t, errStrictTypes)
if tt.isErrFromStrictTypes {
// Ensure coda is **NOT** present.
assert.NotContains(t, errStrictTypes.Error(), strictlyTypedMessageCoda)
} else {
// Ensure coda is present.
assert.ErrorContains(t, errStrictTypes, strictlyTypedMessageCoda)
}
})
}
}
33 changes: 33 additions & 0 deletions otelcol/testdata/weak-empty-map-to-empty-array.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
receivers:
nop:

processors:
nop:

exporters:
nop:

extensions:
nop:

connectors:
nop/con:

service:
telemetry:
metrics:
address: localhost:8888
extensions: [nop]
pipelines:
traces:
receivers: [nop]
processors: {} # <-- Empty map casted to empty array
exporters: [nop, nop/con]
metrics:
receivers: [nop]
processors: [nop]
exporters: [nop]
logs:
receivers: [nop, nop/con]
processors: [nop]
exporters: [nop]
36 changes: 36 additions & 0 deletions otelcol/testdata/weak-implicit-bool-to-int.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
receivers:
nop:

processors:
nop:

exporters:
nop:

extensions:
nop:

connectors:
nop/con:

service:
telemetry:
metrics:
address: localhost:8888
logs:
sampling:
initial: true # <-- Implicit cast bool to int
extensions: [nop]
pipelines:
traces:
receivers: [nop]
processors: [nop]
exporters: [nop, nop/con]
metrics:
receivers: [nop]
processors: [nop]
exporters: [nop]
logs:
receivers: [nop, nop/con]
processors: [nop]
exporters: [nop]
33 changes: 33 additions & 0 deletions otelcol/testdata/weak-implicit-bool-to-string.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
receivers:
nop:

processors:
nop:

exporters:
nop:

extensions:
nop:

connectors:
nop/con:

service:
telemetry:
metrics:
address: true # <-- Implicit cast bool to string
extensions: [nop]
pipelines:
traces:
receivers: [nop]
processors: [nop]
exporters: [nop, nop/con]
metrics:
receivers: [nop]
processors: [nop]
exporters: [nop]
logs:
receivers: [nop, nop/con]
processors: [nop]
exporters: [nop]
36 changes: 36 additions & 0 deletions otelcol/testdata/weak-implicit-int-to-bool.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
receivers:
nop:

processors:
nop:

exporters:
nop:

extensions:
nop:

connectors:
nop/con:

service:
telemetry:
metrics:
address: localhost:8888
logs:
sampling:
enabled: 1 # <-- Implicit cast int to bool
extensions: [nop]
pipelines:
traces:
receivers: [nop]
processors: [nop]
exporters: [nop, nop/con]
metrics:
receivers: [nop]
processors: [nop]
exporters: [nop]
logs:
receivers: [nop, nop/con]
processors: [nop]
exporters: [nop]
33 changes: 33 additions & 0 deletions otelcol/testdata/weak-implicit-int-to-string.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
receivers:
nop:

processors:
nop:

exporters:
nop:

extensions:
nop:

connectors:
nop/con:

service:
telemetry:
metrics:
address: 0xdeadbeef # <-- Implicit cast int to string
extensions: [nop]
pipelines:
traces:
receivers: [nop]
processors: [nop]
exporters: [nop, nop/con]
metrics:
receivers: [nop]
processors: [nop]
exporters: [nop]
logs:
receivers: [nop, nop/con]
processors: [nop]
exporters: [nop]
36 changes: 36 additions & 0 deletions otelcol/testdata/weak-implicit-string-to-bool.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
receivers:
nop:

processors:
nop:

exporters:
nop:

extensions:
nop:

connectors:
nop/con:

service:
telemetry:
metrics:
address: localhost:8888
logs:
sampling:
enabled: t # <-- Implicit cast string to bool
extensions: [nop]
pipelines:
traces:
receivers: [nop]
processors: [nop]
exporters: [nop, nop/con]
metrics:
receivers: [nop]
processors: [nop]
exporters: [nop]
logs:
receivers: [nop, nop/con]
processors: [nop]
exporters: [nop]
Loading

0 comments on commit e7ce1d5

Please sign in to comment.