Skip to content

Commit

Permalink
[pkg/ottl] Improve errors for invalid cache access and add option to …
Browse files Browse the repository at this point in the history
…set context's cache value (#37166)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
- Improved error message for higher context `cache` access. Now when
users try to configure statements like `set(log.attributes["log_key"],
resource.cache["log_key"])`, they will see an error message similar to:

  ```diff
+ access to cache must be be performed using the same statement's
context, please replace "resource.cache[log_key]" with
"log.cache[log_key]"`
- invalid argument at position 1: segment "cache" from path
"resource.cache[log_key]" is not a valid path nor a valid OTTL keyword
for the Resource context - review
https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/pkg/ottl/contexts/ottlresource
to see all valid paths
  ```
- Added the `WithCache` option to all OTTL transformation contexts. This
change allows API consumer to set the initial cache value or share it
among different contexts/executions.

<!-- Issue number (e.g. #1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Split of
#36888

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

<!--Please delete paragraphs that you did not use before submitting.-->
  • Loading branch information
edmocosta authored Jan 13, 2025
1 parent c7a6d57 commit 7e6b19a
Show file tree
Hide file tree
Showing 21 changed files with 319 additions and 36 deletions.
27 changes: 27 additions & 0 deletions .chloggen/ottl-cache-changes.yaml
Original file line number Diff line number Diff line change
@@ -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: pkg/ottl

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Enhanced error messages for invalid cache access and introduced options to configure their values within the OTTL contexts.

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
issues: [29017]

# (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: [user, api]
10 changes: 9 additions & 1 deletion pkg/ottl/contexts/internal/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@

package internal // import "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl/contexts/internal"

import "fmt"
import (
"fmt"
"strings"
)

const (
DefaultErrorMessage = "segment %q from path %q is not a valid path nor a valid OTTL keyword for the %v context - review %v to see all valid paths"
Expand All @@ -20,3 +23,8 @@ const (
func FormatDefaultErrorMessage(pathSegment, fullPath, context, ref string) error {
return fmt.Errorf(DefaultErrorMessage, pathSegment, fullPath, context, ref)
}

func FormatCacheErrorMessage(lowerContext, pathContext, fullPath string) error {
pathSuggestion := strings.Replace(fullPath, pathContext+".", lowerContext+".", 1)
return fmt.Errorf(`access to cache must be performed using the same statement's context, please replace "%s" with "%s"`, fullPath, pathSuggestion)
}
4 changes: 4 additions & 0 deletions pkg/ottl/contexts/internal/path.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ type TestPath[K any] struct {
N string
KeySlice []ottl.Key[K]
NextPath *TestPath[K]
FullPath string
}

func (p *TestPath[K]) Name() string {
Expand All @@ -38,6 +39,9 @@ func (p *TestPath[K]) Keys() []ottl.Key[K] {
}

func (p *TestPath[K]) String() string {
if p.FullPath != "" {
return p.FullPath
}
return p.N
}

Expand Down
4 changes: 3 additions & 1 deletion pkg/ottl/contexts/internal/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ type ResourceContext interface {
GetResourceSchemaURLItem() SchemaURLItem
}

func ResourcePathGetSetter[K ResourceContext](path ottl.Path[K]) (ottl.GetSetter[K], error) {
func ResourcePathGetSetter[K ResourceContext](lowerContext string, path ottl.Path[K]) (ottl.GetSetter[K], error) {
if path == nil {
return nil, FormatDefaultErrorMessage(ResourceContextName, ResourceContextName, "Resource", ResourceContextRef)
}
Expand All @@ -34,6 +34,8 @@ func ResourcePathGetSetter[K ResourceContext](path ottl.Path[K]) (ottl.GetSetter
return accessResourceDroppedAttributesCount[K](), nil
case "schema_url":
return accessResourceSchemaURLItem[K](), nil
case "cache":
return nil, FormatCacheErrorMessage(lowerContext, path.Context(), path.String())
default:
return nil, FormatDefaultErrorMessage(path.Name(), path.String(), "Resource", ResourceContextRef)
}
Expand Down
20 changes: 19 additions & 1 deletion pkg/ottl/contexts/internal/resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.opentelemetry.io/collector/pdata/pcommon"

"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl"
Expand Down Expand Up @@ -311,7 +312,7 @@ func TestResourcePathGetSetter(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
accessor, err := ResourcePathGetSetter[*resourceContext](tt.path)
accessor, err := ResourcePathGetSetter[*resourceContext](tt.path.Context(), tt.path)
assert.NoError(t, err)

resource := createResource()
Expand All @@ -331,6 +332,23 @@ func TestResourcePathGetSetter(t *testing.T) {
}
}

func TestResourcePathGetSetterCacheAccessError(t *testing.T) {
path := &TestPath[*resourceContext]{
N: "cache",
C: "resource",
KeySlice: []ottl.Key[*resourceContext]{
&TestKey[*resourceContext]{
S: ottltest.Strp("key"),
},
},
FullPath: "resource.cache[key]",
}

_, err := ResourcePathGetSetter[*resourceContext]("log", path)
require.Error(t, err)
require.Contains(t, err.Error(), `replace "resource.cache[key]" with "log.cache[key]"`)
}

func createResource() pcommon.Resource {
resource := pcommon.NewResource()
resource.Attributes().PutStr("str", "val")
Expand Down
4 changes: 3 additions & 1 deletion pkg/ottl/contexts/internal/scope.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ type InstrumentationScopeContext interface {
GetScopeSchemaURLItem() SchemaURLItem
}

func ScopePathGetSetter[K InstrumentationScopeContext](path ottl.Path[K]) (ottl.GetSetter[K], error) {
func ScopePathGetSetter[K InstrumentationScopeContext](lowerContext string, path ottl.Path[K]) (ottl.GetSetter[K], error) {
if path == nil {
return nil, FormatDefaultErrorMessage(InstrumentationScopeContextName, InstrumentationScopeContextName, "Instrumentation Scope", InstrumentationScopeRef)
}
Expand All @@ -40,6 +40,8 @@ func ScopePathGetSetter[K InstrumentationScopeContext](path ottl.Path[K]) (ottl.
return accessInstrumentationScopeDroppedAttributesCount[K](), nil
case "schema_url":
return accessInstrumentationScopeSchemaURLItem[K](), nil
case "cache":
return nil, FormatCacheErrorMessage(lowerContext, path.Context(), path.String())
default:
return nil, FormatDefaultErrorMessage(path.Name(), path.String(), "Instrumentation Scope", InstrumentationScopeRef)
}
Expand Down
20 changes: 19 additions & 1 deletion pkg/ottl/contexts/internal/scope_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.opentelemetry.io/collector/pdata/pcommon"

"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl"
Expand Down Expand Up @@ -349,7 +350,7 @@ func TestScopePathGetSetter(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
accessor, err := ScopePathGetSetter[*instrumentationScopeContext](tt.path)
accessor, err := ScopePathGetSetter[*instrumentationScopeContext](tt.path.Context(), tt.path)
assert.NoError(t, err)

is := createInstrumentationScope()
Expand All @@ -369,6 +370,23 @@ func TestScopePathGetSetter(t *testing.T) {
}
}

func TestScopePathGetSetterCacheAccessError(t *testing.T) {
path := &TestPath[*instrumentationScopeContext]{
N: "cache",
C: "instrumentation_scope",
KeySlice: []ottl.Key[*instrumentationScopeContext]{
&TestKey[*instrumentationScopeContext]{
S: ottltest.Strp("key"),
},
},
FullPath: "instrumentation_scope.cache[key]",
}

_, err := ScopePathGetSetter[*instrumentationScopeContext]("metric", path)
require.Error(t, err)
require.Contains(t, err.Error(), `replace "instrumentation_scope.cache[key]" with "metric.cache[key]"`)
}

func createInstrumentationScope() pcommon.InstrumentationScope {
is := pcommon.NewInstrumentationScope()
is.SetName("library")
Expand Down
4 changes: 3 additions & 1 deletion pkg/ottl/contexts/internal/span.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ var SpanSymbolTable = map[ottl.EnumSymbol]ottl.Enum{
"STATUS_CODE_ERROR": ottl.Enum(ptrace.StatusCodeError),
}

func SpanPathGetSetter[K SpanContext](path ottl.Path[K]) (ottl.GetSetter[K], error) {
func SpanPathGetSetter[K SpanContext](lowerContext string, path ottl.Path[K]) (ottl.GetSetter[K], error) {
if path == nil {
return nil, FormatDefaultErrorMessage(SpanContextName, SpanContextName, SpanContextNameDescription, SpanRef)
}
Expand Down Expand Up @@ -128,6 +128,8 @@ func SpanPathGetSetter[K SpanContext](path ottl.Path[K]) (ottl.GetSetter[K], err
}
}
return accessStatus[K](), nil
case "cache":
return nil, FormatCacheErrorMessage(lowerContext, path.Context(), path.String())
default:
return nil, FormatDefaultErrorMessage(path.Name(), path.String(), SpanContextNameDescription, SpanRef)
}
Expand Down
20 changes: 19 additions & 1 deletion pkg/ottl/contexts/internal/span_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.opentelemetry.io/collector/pdata/pcommon"
"go.opentelemetry.io/collector/pdata/ptrace"

Expand Down Expand Up @@ -603,7 +604,7 @@ func TestSpanPathGetSetter(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
accessor, err := SpanPathGetSetter[*spanContext](tt.path)
accessor, err := SpanPathGetSetter[*spanContext](tt.path.Context(), tt.path)
assert.NoError(t, err)

span := createSpan()
Expand All @@ -623,6 +624,23 @@ func TestSpanPathGetSetter(t *testing.T) {
}
}

func TestSpanPathGetSetterCacheAccessError(t *testing.T) {
path := &TestPath[*spanContext]{
N: "cache",
C: "span",
KeySlice: []ottl.Key[*spanContext]{
&TestKey[*spanContext]{
S: ottltest.Strp("key"),
},
},
FullPath: "span.cache[key]",
}

_, err := SpanPathGetSetter[*spanContext]("spanevent", path)
require.Error(t, err)
require.Contains(t, err.Error(), `replace "span.cache[key]" with "spanevent.cache[key]"`)
}

func createSpan() ptrace.Span {
span := ptrace.NewSpan()
span.SetTraceID(traceID)
Expand Down
23 changes: 19 additions & 4 deletions pkg/ottl/contexts/ottldatapoint/datapoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,10 @@ func (tCtx TransformContext) MarshalLogObject(encoder zapcore.ObjectEncoder) err

type Option func(*ottl.Parser[TransformContext])

func NewTransformContext(dataPoint any, metric pmetric.Metric, metrics pmetric.MetricSlice, instrumentationScope pcommon.InstrumentationScope, resource pcommon.Resource, scopeMetrics pmetric.ScopeMetrics, resourceMetrics pmetric.ResourceMetrics) TransformContext {
return TransformContext{
type TransformContextOption func(*TransformContext)

func NewTransformContext(dataPoint any, metric pmetric.Metric, metrics pmetric.MetricSlice, instrumentationScope pcommon.InstrumentationScope, resource pcommon.Resource, scopeMetrics pmetric.ScopeMetrics, resourceMetrics pmetric.ResourceMetrics, options ...TransformContextOption) TransformContext {
tc := TransformContext{
dataPoint: dataPoint,
metric: metric,
metrics: metrics,
Expand All @@ -75,6 +77,19 @@ func NewTransformContext(dataPoint any, metric pmetric.Metric, metrics pmetric.M
scopeMetrics: scopeMetrics,
resourceMetrics: resourceMetrics,
}
for _, opt := range options {
opt(&tc)
}
return tc
}

// Experimental: *NOTE* this option is subject to change or removal in the future.
func WithCache(cache *pcommon.Map) TransformContextOption {
return func(p *TransformContext) {
if cache != nil {
p.cache = *cache
}
}
}

func (tCtx TransformContext) GetDataPoint() any {
Expand Down Expand Up @@ -289,9 +304,9 @@ func (pep *pathExpressionParser) parsePath(path ottl.Path[TransformContext]) (ot
func (pep *pathExpressionParser) parseHigherContextPath(context string, path ottl.Path[TransformContext]) (ottl.GetSetter[TransformContext], error) {
switch context {
case internal.ResourceContextName:
return internal.ResourcePathGetSetter(path)
return internal.ResourcePathGetSetter(ContextName, path)
case internal.InstrumentationScopeContextName:
return internal.ScopePathGetSetter(path)
return internal.ScopePathGetSetter(ContextName, path)
case internal.MetricContextName:
return internal.MetricPathGetSetter(path)
default:
Expand Down
18 changes: 18 additions & 0 deletions pkg/ottl/contexts/ottldatapoint/datapoint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,24 @@ func Test_newPathGetSetter_Cache(t *testing.T) {
}
}

func Test_newPathGetSetter_WithCache(t *testing.T) {
cacheValue := pcommon.NewMap()
cacheValue.PutStr("test", "pass")

ctx := NewTransformContext(
pmetric.NewNumberDataPoint(),
pmetric.NewMetric(),
pmetric.NewMetricSlice(),
pcommon.NewInstrumentationScope(),
pcommon.NewResource(),
pmetric.NewScopeMetrics(),
pmetric.NewResourceMetrics(),
WithCache(&cacheValue),
)

assert.Equal(t, cacheValue, ctx.getCache())
}

func Test_newPathGetSetter_NumberDataPoint(t *testing.T) {
refNumberDataPoint := createNumberDataPointTelemetry(pmetric.NumberDataPointValueTypeInt)

Expand Down
23 changes: 19 additions & 4 deletions pkg/ottl/contexts/ottllog/log.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,15 +71,30 @@ func (tCtx TransformContext) MarshalLogObject(encoder zapcore.ObjectEncoder) err

type Option func(*ottl.Parser[TransformContext])

func NewTransformContext(logRecord plog.LogRecord, instrumentationScope pcommon.InstrumentationScope, resource pcommon.Resource, scopeLogs plog.ScopeLogs, resourceLogs plog.ResourceLogs) TransformContext {
return TransformContext{
type TransformContextOption func(*TransformContext)

func NewTransformContext(logRecord plog.LogRecord, instrumentationScope pcommon.InstrumentationScope, resource pcommon.Resource, scopeLogs plog.ScopeLogs, resourceLogs plog.ResourceLogs, options ...TransformContextOption) TransformContext {
tc := TransformContext{
logRecord: logRecord,
instrumentationScope: instrumentationScope,
resource: resource,
cache: pcommon.NewMap(),
scopeLogs: scopeLogs,
resourceLogs: resourceLogs,
}
for _, opt := range options {
opt(&tc)
}
return tc
}

// Experimental: *NOTE* this option is subject to change or removal in the future.
func WithCache(cache *pcommon.Map) TransformContextOption {
return func(p *TransformContext) {
if cache != nil {
p.cache = *cache
}
}
}

func (tCtx TransformContext) GetLogRecord() plog.LogRecord {
Expand Down Expand Up @@ -290,9 +305,9 @@ func (pep *pathExpressionParser) parsePath(path ottl.Path[TransformContext]) (ot
func (pep *pathExpressionParser) parseHigherContextPath(context string, path ottl.Path[TransformContext]) (ottl.GetSetter[TransformContext], error) {
switch context {
case internal.ResourceContextName:
return internal.ResourcePathGetSetter(path)
return internal.ResourcePathGetSetter(ContextName, path)
case internal.InstrumentationScopeContextName:
return internal.ScopePathGetSetter(path)
return internal.ScopePathGetSetter(ContextName, path)
default:
var fullPath string
if path != nil {
Expand Down
16 changes: 16 additions & 0 deletions pkg/ottl/contexts/ottllog/log_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -694,6 +694,22 @@ func Test_newPathGetSetter_higherContextPath(t *testing.T) {
}
}

func Test_newPathGetSetter_WithCache(t *testing.T) {
cacheValue := pcommon.NewMap()
cacheValue.PutStr("test", "pass")

ctx := NewTransformContext(
plog.NewLogRecord(),
pcommon.NewInstrumentationScope(),
pcommon.NewResource(),
plog.NewScopeLogs(),
plog.NewResourceLogs(),
WithCache(&cacheValue),
)

assert.Equal(t, cacheValue, ctx.getCache())
}

func createTelemetry(bodyType string) (plog.LogRecord, pcommon.InstrumentationScope, pcommon.Resource) {
log := plog.NewLogRecord()
log.SetTimestamp(pcommon.NewTimestampFromTime(time.UnixMilli(100)))
Expand Down
Loading

0 comments on commit 7e6b19a

Please sign in to comment.