Skip to content

Commit 758ae55

Browse files
edmocostaAkshayS198
authored andcommitted
[processor/transform] Fix basic config cache access (open-telemetry#39290)
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description This PR removes the shared cache logic we introduced to support Basic Config statements, and changes the approach to group all Basic config statements together into the same sharing-cache `common.ContextStatements`, embracing the limitation of not being able to infer the context depending on the user's statements (similar to the advanced config). To help with that limitation, the context inferrer validation was improved, and it now returns specific errors that better describe why it couldn't infer a valid context. This PR also introduces a **breaking change**, that consists in forcing users to use either basic or advanced configuration, for example, mixed configurations like the following won't be valid anymore, and it will require users to use only one style: ```yaml log_statements: - set(resource.attributes["foo"], "foo") - statements: - set(resource.attributes["bar"], "bar") ``` --- Another PR will be open for removing the `WithCache` option from all OTTL contexts. <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes open-telemetry#38926 <!--Describe what testing was performed and which tests were added.--> #### Testing Unit tests <!--Describe the documentation added.--> #### Documentation Updated README <!--Please delete paragraphs that you did not use before submitting.-->
1 parent fec31f1 commit 758ae55

21 files changed

+291
-411
lines changed
Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
# Use this changelog template to create an entry for release notes.
2+
3+
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
4+
change_type: breaking
5+
6+
# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
7+
component: processor/transform
8+
9+
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
10+
note: Fix Basic Config style to properly handle `cache` access.
11+
12+
# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
13+
issues: [38926]
14+
15+
# (Optional) One or more lines of additional information to render under the primary note.
16+
# These lines will be padded with 2 spaces and then inserted directly into the document.
17+
# Use pipe (|) for multiline entries.
18+
subtext:
19+
The Transform processor now requires only one configuration style per processor's configuration,
20+
which means Advanced Config and Basic Config cannot be used together anymore.
21+
22+
# If your change doesn't affect end users or the exported elements of any package,
23+
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
24+
# Optional: The change log or logs in which this entry should be included.
25+
# e.g. '[user]' or '[user, api]'
26+
# Include 'user' if the change is relevant to end users.
27+
# Include 'api' if there is a change to a library API.
28+
# Default: '[user]'
29+
change_logs: []

pkg/ottl/context_inferrer.go

Lines changed: 30 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ package ottl // import "github.com/open-telemetry/opentelemetry-collector-contri
55

66
import (
77
"cmp"
8+
"errors"
89
"fmt"
910
"math"
1011
"slices"
@@ -105,7 +106,7 @@ func (s *priorityContextInferrer) infer(ottls []string, hinter hinterFunc) (infe
105106
if inferredContext != "" {
106107
s.telemetrySettings.Logger.Debug(fmt.Sprintf(`Inferred context: "%s"`, inferredContext))
107108
} else {
108-
s.telemetrySettings.Logger.Debug("Unable to infer context from statements")
109+
s.telemetrySettings.Logger.Debug("Unable to infer context from statements", zap.Error(err))
109110
}
110111
}()
111112

@@ -114,9 +115,9 @@ func (s *priorityContextInferrer) infer(ottls []string, hinter hinterFunc) (infe
114115

115116
var inferredContextPriority int
116117
for _, ottl := range ottls {
117-
ottlPaths, ottlFunctions, ottlEnums, err := hinter(ottl)
118-
if err != nil {
119-
return "", err
118+
ottlPaths, ottlFunctions, ottlEnums, hinterErr := hinter(ottl)
119+
if hinterErr != nil {
120+
return "", hinterErr
120121
}
121122
for _, p := range ottlPaths {
122123
candidate := p.Context
@@ -141,11 +142,13 @@ func (s *priorityContextInferrer) infer(ottls []string, hinter hinterFunc) (infe
141142
s.telemetrySettings.Logger.Debug("No context candidate found in the ottls")
142143
return inferredContext, nil
143144
}
144-
ok := s.validateContextCandidate(inferredContext, requiredFunctions, requiredEnums)
145-
if ok {
145+
if err = s.validateContextCandidate(inferredContext, requiredFunctions, requiredEnums); err == nil {
146146
return inferredContext, nil
147147
}
148-
return s.inferFromLowerContexts(inferredContext, requiredFunctions, requiredEnums), nil
148+
if inferredFromLowerContexts, lowerContextErr := s.inferFromLowerContexts(inferredContext, requiredFunctions, requiredEnums); lowerContextErr == nil {
149+
return inferredFromLowerContexts, nil
150+
}
151+
return "", err
149152
}
150153

151154
// validateContextCandidate checks if the given context candidate has all required functions names
@@ -154,29 +157,26 @@ func (s *priorityContextInferrer) validateContextCandidate(
154157
context string,
155158
requiredFunctions map[string]struct{},
156159
requiredEnums map[enumSymbol]struct{},
157-
) bool {
160+
) error {
158161
s.telemetrySettings.Logger.Debug(fmt.Sprintf(`Validating selected context candidate: "%s"`, context))
159162
candidate, ok := s.contextCandidate[context]
160163
if !ok {
161-
s.telemetrySettings.Logger.Debug(fmt.Sprintf(`Context "%s" is not a valid candidate`, context))
162-
return false
164+
return fmt.Errorf(`inferred context "%s" is not a valid candidate`, context)
163165
}
164166
if len(requiredFunctions) == 0 && len(requiredEnums) == 0 {
165-
return true
167+
return nil
166168
}
167169
for function := range requiredFunctions {
168170
if !candidate.hasFunctionName(function) {
169-
s.telemetrySettings.Logger.Debug(fmt.Sprintf(`Context "%s" does not meet the function requirement: "%s"`, context, function))
170-
return false
171+
return fmt.Errorf(`inferred context "%s" does not support the function "%s"`, context, function)
171172
}
172173
}
173174
for enum := range requiredEnums {
174175
if !candidate.hasEnumSymbol((*EnumSymbol)(&enum)) {
175-
s.telemetrySettings.Logger.Debug(fmt.Sprintf(`Context "%s" does not meet the enum requirement: "%s"`, context, string(enum)))
176-
return false
176+
return fmt.Errorf(`inferred context "%s" does not support the enum symbol "%s"`, context, string(enum))
177177
}
178178
}
179-
return true
179+
return nil
180180
}
181181

182182
// inferFromLowerContexts returns the first lower context that supports all required functions
@@ -187,26 +187,33 @@ func (s *priorityContextInferrer) inferFromLowerContexts(
187187
context string,
188188
requiredFunctions map[string]struct{},
189189
requiredEnums map[enumSymbol]struct{},
190-
) string {
190+
) (inferredContext string, err error) {
191191
s.telemetrySettings.Logger.Debug(fmt.Sprintf(`Trying to infer context using "%s" lower contexts`, context))
192+
193+
defer func() {
194+
if err != nil {
195+
s.telemetrySettings.Logger.Debug("Unable to infer context from lower contexts", zap.Error(err))
196+
}
197+
}()
198+
192199
inferredContextCandidate, ok := s.contextCandidate[context]
193200
if !ok {
194-
return ""
201+
return "", fmt.Errorf(`context "%s" is not a valid candidate`, context)
195202
}
196203

197204
lowerContextCandidates := inferredContextCandidate.getLowerContexts(context)
198205
if len(lowerContextCandidates) == 0 {
199-
return ""
206+
return "", fmt.Errorf(`context "%s" has no lower contexts candidates`, context)
200207
}
201208

202209
s.sortContextCandidates(lowerContextCandidates)
203210
for _, lowerCandidate := range lowerContextCandidates {
204-
ok = s.validateContextCandidate(lowerCandidate, requiredFunctions, requiredEnums)
205-
if ok {
206-
return lowerCandidate
211+
if candidateErr := s.validateContextCandidate(lowerCandidate, requiredFunctions, requiredEnums); candidateErr == nil {
212+
return lowerCandidate, nil
207213
}
214+
s.telemetrySettings.Logger.Debug(fmt.Sprintf(`lower context "%s" is not a valid candidate`, lowerCandidate), zap.Error(err))
208215
}
209-
return ""
216+
return "", errors.New("no valid lower context found")
210217
}
211218

212219
// sortContextCandidates sorts the slice candidates using the priorityContextInferrer.contextsPriority order.

pkg/ottl/context_inferrer_test.go

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ func Test_NewPriorityContextInferrer_InferStatements(t *testing.T) {
4444
candidates map[string]*priorityContextInferrerCandidate
4545
statements []string
4646
expected string
47+
err string
4748
}{
4849
{
4950
name: "with priority and statement context",
@@ -109,7 +110,7 @@ func Test_NewPriorityContextInferrer_InferStatements(t *testing.T) {
109110
"metric": newDummyPriorityContextInferrerCandidate(false, false, []string{"datapoint"}),
110111
"datapoint": newDummyPriorityContextInferrerCandidate(false, false, []string{}),
111112
},
112-
expected: "",
113+
err: `inferred context "metric" does not support the function "set"`,
113114
},
114115
{
115116
name: "inferred path context with missing function and no lower context",
@@ -118,7 +119,7 @@ func Test_NewPriorityContextInferrer_InferStatements(t *testing.T) {
118119
candidates: map[string]*priorityContextInferrerCandidate{
119120
"metric": newDummyPriorityContextInferrerCandidate(false, true, []string{}),
120121
},
121-
expected: "",
122+
err: `inferred context "metric" does not support the function "set"`,
122123
},
123124
{
124125
name: "inferred path context with missing enum",
@@ -135,7 +136,7 @@ func Test_NewPriorityContextInferrer_InferStatements(t *testing.T) {
135136
priority: []string{"unknown"},
136137
statements: []string{`set(unknown.count, 0)`},
137138
candidates: map[string]*priorityContextInferrerCandidate{},
138-
expected: "",
139+
err: `inferred context "unknown" is not a valid candidate`,
139140
},
140141
}
141142

@@ -147,6 +148,11 @@ func Test_NewPriorityContextInferrer_InferStatements(t *testing.T) {
147148
withContextInferrerPriorities(tt.priority),
148149
)
149150
inferredContext, err := inferrer.inferFromStatements(tt.statements)
151+
if tt.err != "" {
152+
require.ErrorContains(t, err, tt.err)
153+
return
154+
}
155+
150156
require.NoError(t, err)
151157
assert.Equal(t, tt.expected, inferredContext)
152158
})

pkg/ottl/parser_collection.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,7 @@ func (pc *ParserCollection[R]) ParseStatements(statements StatementsGetter) (R,
333333
statementsValues := statements.GetStatements()
334334
inferredContext, err := pc.contextInferrer.inferFromStatements(statementsValues)
335335
if err != nil {
336-
return *new(R), err
336+
return *new(R), fmt.Errorf("unable to infer a valid context (%+q) from statements %+q: %w", pc.supportedContextNames(), statementsValues, err)
337337
}
338338

339339
if inferredContext == "" {

pkg/ottl/parser_collection_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -300,18 +300,18 @@ func Test_ParseStatements_NoContextInferredError(t *testing.T) {
300300
statements := mockGetter{values: []string{`set(bar.attributes["bar"], "foo")`}}
301301
_, err = pc.ParseStatements(statements)
302302

303-
assert.ErrorContains(t, err, "unable to infer context from statements")
303+
assert.ErrorContains(t, err, "unable to infer a valid context")
304304
}
305305

306306
func Test_ParseStatements_ContextInferenceError(t *testing.T) {
307-
pc, err := NewParserCollection[any](componenttest.NewNopTelemetrySettings())
307+
pc, err := NewParserCollection[any](componenttest.NewNopTelemetrySettings(), WithParserCollectionContext[any, any]("foo", mockParser(t, WithPathContextNames[any]([]string{"foo"})), WithStatementConverter(newNopParsedStatementsConverter[any]())))
308308
require.NoError(t, err)
309309
pc.contextInferrer = &mockFailingContextInferrer{err: errors.New("inference error")}
310310

311311
statements := mockGetter{values: []string{`set(bar.attributes["bar"], "foo")`}}
312312
_, err = pc.ParseStatements(statements)
313313

314-
assert.EqualError(t, err, "inference error")
314+
assert.ErrorContains(t, err, "inference error")
315315
}
316316

317317
func Test_ParseStatements_UnknownContextError(t *testing.T) {

processor/transformprocessor/README.md

Lines changed: 28 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,9 @@ transform:
112112
- set(log.body, log.attributes["http.route"])
113113
```
114114

115-
If you're interested in how OTTL parses these statements, see [Context Inference](#context-inference).
115+
In some situations a combination of Paths, functions, or enums is not allowed, and the solution
116+
might require multiple [Advanced Config](#advanced-config) configuration groups.
117+
See [Context Inference](#context-inference) for more details.
116118

117119
### Advanced Config
118120

@@ -171,37 +173,8 @@ transform:
171173
```
172174

173175
The Transform Processor will enforce that all the Paths, functions, and enums used in a group's `statements` are parsable.
174-
In some situations a combination of Paths, functions, or enums is not allowed. For example:
175-
176-
```yaml
177-
metric_statements:
178-
- statements:
179-
- convert_sum_to_gauge() where metric.name == "system.processes.count"
180-
- limit(datapoint.attributes, 100, ["host.name"])
181-
```
182-
183-
In this configuration, the `datapoint` Path prefixed is used in the same group of statements as the `convert_sum_to_gauge`
184-
function. Since `convert_sum_to_gauge` can only be used with the metrics, not datapoints, but the list
185-
statements contains a reference to the datapoints via the `datapoint` Path prefix, the group of statements cannot
186-
be parsed.
187-
188-
The solution is to separate the statements into separate groups:
189-
190-
```yaml
191-
metric_statements:
192-
- statements:
193-
- limit(datapoint.attributes, 100, ["host.name"])
194-
- statements:
195-
- convert_sum_to_gauge() where metric.name == "system.processes.count"
196-
```
197-
198-
Alternatively, for simplicity, you can use the [basic configuration](#basic-config) style:
199-
200-
```yaml
201-
metric_statements:
202-
- limit(datapoint.attributes, 100, ["host.name"])
203-
- convert_sum_to_gauge() where metric.name == "system.processes.count"
204-
```
176+
In some situations a combination of Paths, functions, or enums is not allowed, and it might require multiple configuration groups.
177+
See [Context Inference](#context-inference) for more details.
205178

206179
### Context inference
207180

@@ -239,6 +212,29 @@ are both accurate and performant, leveraging the hierarchical structure of conte
239212
iterations and improve overall processing efficiency.
240213
All of this happens automatically, leaving you to write OTTL statements without worrying about Context.
241214

215+
In some situations a combination of Paths, functions, or enums is not allowed. For example:
216+
217+
```yaml
218+
metric_statements:
219+
- convert_sum_to_gauge() where metric.name == "system.processes.count"
220+
- limit(datapoint.attributes, 100, ["host.name"])
221+
```
222+
223+
In this configuration, the `datapoint` Path prefixed is used in the same group of statements as the `convert_sum_to_gauge`
224+
function. Since `convert_sum_to_gauge` can only be used with the metrics, not datapoints, but the list
225+
statements contains a reference to the datapoints via the `datapoint` Path prefix, the group of statements cannot
226+
be parsed.
227+
228+
The solution is to separate the statements into separate [Advanced Config](#advanced-config) groups:
229+
230+
```yaml
231+
metric_statements:
232+
- statements:
233+
- convert_sum_to_gauge() where metric.name == "system.processes.count"
234+
- statements:
235+
- limit(datapoint.attributes, 100, ["host.name"])
236+
```
237+
242238
## Grammar
243239

244240
You can learn more in-depth details on the capabilities and limitations of the OpenTelemetry Transformation Language used by the Transform Processor by reading about its [grammar](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/pkg/ottl/LANGUAGE.md).

processor/transformprocessor/config.go

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,6 @@ func (c *Config) Unmarshal(conf *confmap.Conf) error {
7878
"log_statements": &c.LogStatements,
7979
}
8080

81-
flatContextStatements := map[string][]int{}
8281
contextStatementsPatch := map[string]any{}
8382
for fieldName := range contextStatementsFields {
8483
if !conf.IsSet(fieldName) {
@@ -93,17 +92,25 @@ func (c *Config) Unmarshal(conf *confmap.Conf) error {
9392
continue
9493
}
9594

96-
stmts := make([]any, 0, len(values))
97-
for i, value := range values {
98-
// Array of strings means it's a flat configuration style
95+
statementsConfigs := make([]any, 0, len(values))
96+
var basicStatements []any
97+
for _, value := range values {
98+
// Array of strings means it's a basic configuration style
9999
if reflect.TypeOf(value).Kind() == reflect.String {
100-
stmts = append(stmts, map[string]any{"statements": []any{value}})
101-
flatContextStatements[fieldName] = append(flatContextStatements[fieldName], i)
100+
basicStatements = append(basicStatements, value)
102101
} else {
103-
stmts = append(stmts, value)
102+
if len(basicStatements) > 0 {
103+
return errors.New("configuring multiple configuration styles is not supported, please use only Basic configuration or only Advanced configuration")
104+
}
105+
statementsConfigs = append(statementsConfigs, value)
104106
}
105107
}
106-
contextStatementsPatch[fieldName] = stmts
108+
109+
if len(basicStatements) > 0 {
110+
statementsConfigs = append(statementsConfigs, map[string]any{"statements": basicStatements})
111+
}
112+
113+
contextStatementsPatch[fieldName] = statementsConfigs
107114
}
108115

109116
if len(contextStatementsPatch) > 0 {
@@ -118,12 +125,6 @@ func (c *Config) Unmarshal(conf *confmap.Conf) error {
118125
return err
119126
}
120127

121-
for fieldName, indexes := range flatContextStatements {
122-
for _, i := range indexes {
123-
(*contextStatementsFields[fieldName])[i].SharedCache = true
124-
}
125-
}
126-
127128
return err
128129
}
129130

0 commit comments

Comments
 (0)