Skip to content

Commit

Permalink
Apply PR suggestions (better cache messages, support error mode per s…
Browse files Browse the repository at this point in the history
…tatements, conditions, etc)
  • Loading branch information
edmocosta committed Jan 10, 2025
1 parent 79128db commit b241fdd
Show file tree
Hide file tree
Showing 31 changed files with 1,390 additions and 322 deletions.
49 changes: 42 additions & 7 deletions internal/filter/filterottl/filter.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,12 @@ import (
// The passed in functions should use the ottlspan.TransformContext.
// If a function named `match` is not present in the function map it will be added automatically so that parsing works as expected
func NewBoolExprForSpan(conditions []string, functions map[string]ottl.Factory[ottlspan.TransformContext], errorMode ottl.ErrorMode, set component.TelemetrySettings) (*ottl.ConditionSequence[ottlspan.TransformContext], error) {
parser, err := ottlspan.NewParser(functions, set)
return NewBoolExprForSpanWithOptions(conditions, functions, errorMode, set, nil)
}

// NewBoolExprForSpanWithOptions is like NewBoolExprForSpan, but with additional options.
func NewBoolExprForSpanWithOptions(conditions []string, functions map[string]ottl.Factory[ottlspan.TransformContext], errorMode ottl.ErrorMode, set component.TelemetrySettings, parserOptions []ottlspan.Option) (*ottl.ConditionSequence[ottlspan.TransformContext], error) {
parser, err := ottlspan.NewParser(functions, set, parserOptions...)
if err != nil {
return nil, err
}
Expand All @@ -36,7 +41,12 @@ func NewBoolExprForSpan(conditions []string, functions map[string]ottl.Factory[o
// The passed in functions should use the ottlspanevent.TransformContext.
// If a function named `match` is not present in the function map it will be added automatically so that parsing works as expected
func NewBoolExprForSpanEvent(conditions []string, functions map[string]ottl.Factory[ottlspanevent.TransformContext], errorMode ottl.ErrorMode, set component.TelemetrySettings) (*ottl.ConditionSequence[ottlspanevent.TransformContext], error) {
parser, err := ottlspanevent.NewParser(functions, set)
return NewBoolExprForSpanEventWithOptions(conditions, functions, errorMode, set, nil)
}

// NewBoolExprForSpanEventWithOptions is like NewBoolExprForSpanEvent, but with additional options.
func NewBoolExprForSpanEventWithOptions(conditions []string, functions map[string]ottl.Factory[ottlspanevent.TransformContext], errorMode ottl.ErrorMode, set component.TelemetrySettings, parserOptions []ottlspanevent.Option) (*ottl.ConditionSequence[ottlspanevent.TransformContext], error) {
parser, err := ottlspanevent.NewParser(functions, set, parserOptions...)
if err != nil {
return nil, err
}
Expand All @@ -52,7 +62,12 @@ func NewBoolExprForSpanEvent(conditions []string, functions map[string]ottl.Fact
// The passed in functions should use the ottlmetric.TransformContext.
// If a function named `match` is not present in the function map it will be added automatically so that parsing works as expected
func NewBoolExprForMetric(conditions []string, functions map[string]ottl.Factory[ottlmetric.TransformContext], errorMode ottl.ErrorMode, set component.TelemetrySettings) (*ottl.ConditionSequence[ottlmetric.TransformContext], error) {
parser, err := ottlmetric.NewParser(functions, set)
return NewBoolExprForMetricWithOptions(conditions, functions, errorMode, set, nil)
}

// NewBoolExprForMetricWithOptions is like NewBoolExprForMetric, but with additional options.
func NewBoolExprForMetricWithOptions(conditions []string, functions map[string]ottl.Factory[ottlmetric.TransformContext], errorMode ottl.ErrorMode, set component.TelemetrySettings, parserOptions []ottlmetric.Option) (*ottl.ConditionSequence[ottlmetric.TransformContext], error) {
parser, err := ottlmetric.NewParser(functions, set, parserOptions...)
if err != nil {
return nil, err
}
Expand All @@ -68,7 +83,12 @@ func NewBoolExprForMetric(conditions []string, functions map[string]ottl.Factory
// The passed in functions should use the ottldatapoint.TransformContext.
// If a function named `match` is not present in the function map it will be added automatically so that parsing works as expected
func NewBoolExprForDataPoint(conditions []string, functions map[string]ottl.Factory[ottldatapoint.TransformContext], errorMode ottl.ErrorMode, set component.TelemetrySettings) (*ottl.ConditionSequence[ottldatapoint.TransformContext], error) {
parser, err := ottldatapoint.NewParser(functions, set)
return NewBoolExprForDataPointWithOptions(conditions, functions, errorMode, set, nil)
}

// NewBoolExprForDataPointWithOptions is like NewBoolExprForDataPoint, but with additional options.
func NewBoolExprForDataPointWithOptions(conditions []string, functions map[string]ottl.Factory[ottldatapoint.TransformContext], errorMode ottl.ErrorMode, set component.TelemetrySettings, parserOptions []ottldatapoint.Option) (*ottl.ConditionSequence[ottldatapoint.TransformContext], error) {
parser, err := ottldatapoint.NewParser(functions, set, parserOptions...)
if err != nil {
return nil, err
}
Expand All @@ -84,7 +104,12 @@ func NewBoolExprForDataPoint(conditions []string, functions map[string]ottl.Fact
// The passed in functions should use the ottllog.TransformContext.
// If a function named `match` is not present in the function map it will be added automatically so that parsing works as expected
func NewBoolExprForLog(conditions []string, functions map[string]ottl.Factory[ottllog.TransformContext], errorMode ottl.ErrorMode, set component.TelemetrySettings) (*ottl.ConditionSequence[ottllog.TransformContext], error) {
parser, err := ottllog.NewParser(functions, set)
return NewBoolExprForLogWithOptions(conditions, functions, errorMode, set, nil)
}

// NewBoolExprForLogWithOptions is like NewBoolExprForLog, but with additional options.
func NewBoolExprForLogWithOptions(conditions []string, functions map[string]ottl.Factory[ottllog.TransformContext], errorMode ottl.ErrorMode, set component.TelemetrySettings, parserOptions []ottllog.Option) (*ottl.ConditionSequence[ottllog.TransformContext], error) {
parser, err := ottllog.NewParser(functions, set, parserOptions...)
if err != nil {
return nil, err
}
Expand All @@ -100,7 +125,12 @@ func NewBoolExprForLog(conditions []string, functions map[string]ottl.Factory[ot
// The passed in functions should use the ottlresource.TransformContext.
// If a function named `match` is not present in the function map it will be added automatically so that parsing works as expected
func NewBoolExprForResource(conditions []string, functions map[string]ottl.Factory[ottlresource.TransformContext], errorMode ottl.ErrorMode, set component.TelemetrySettings) (*ottl.ConditionSequence[ottlresource.TransformContext], error) {
parser, err := ottlresource.NewParser(functions, set)
return NewBoolExprForResourceWithOptions(conditions, functions, errorMode, set, nil)
}

// NewBoolExprForResourceWithOptions is like NewBoolExprForResource, but with additional options.
func NewBoolExprForResourceWithOptions(conditions []string, functions map[string]ottl.Factory[ottlresource.TransformContext], errorMode ottl.ErrorMode, set component.TelemetrySettings, parserOptions []ottlresource.Option) (*ottl.ConditionSequence[ottlresource.TransformContext], error) {
parser, err := ottlresource.NewParser(functions, set, parserOptions...)
if err != nil {
return nil, err
}
Expand All @@ -116,7 +146,12 @@ func NewBoolExprForResource(conditions []string, functions map[string]ottl.Facto
// The passed in functions should use the ottlresource.TransformContext.
// If a function named `match` is not present in the function map it will be added automatically so that parsing works as expected
func NewBoolExprForScope(conditions []string, functions map[string]ottl.Factory[ottlscope.TransformContext], errorMode ottl.ErrorMode, set component.TelemetrySettings) (*ottl.ConditionSequence[ottlscope.TransformContext], error) {
parser, err := ottlscope.NewParser(functions, set)
return NewBoolExprForScopeWithOptions(conditions, functions, errorMode, set, nil)
}

// NewBoolExprForScopeWithOptions is like NewBoolExprForScope, but with additional options.
func NewBoolExprForScopeWithOptions(conditions []string, functions map[string]ottl.Factory[ottlscope.TransformContext], errorMode ottl.ErrorMode, set component.TelemetrySettings, parserOptions []ottlscope.Option) (*ottl.ConditionSequence[ottlscope.TransformContext], error) {
parser, err := ottlscope.NewParser(functions, set, parserOptions...)
if err != nil {
return nil, err
}
Expand Down
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" by "%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]" by "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]" by "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]" by "spanevent.cache[key]"`)
}

func createSpan() ptrace.Span {
span := ptrace.NewSpan()
span.SetTraceID(traceID)
Expand Down
28 changes: 19 additions & 9 deletions pkg/ottl/contexts/ottldatapoint/datapoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,22 +64,32 @@ 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 NewTransformContextWithCache(dataPoint, metric, metrics, instrumentationScope, resource, scopeMetrics, resourceMetrics, pcommon.NewMap())
}
type TransformContextOption func(*TransformContext)

// Experimental: *NOTE* this function is subject to change or removal in the future.
func NewTransformContextWithCache(dataPoint any, metric pmetric.Metric, metrics pmetric.MetricSlice, instrumentationScope pcommon.InstrumentationScope, resource pcommon.Resource, scopeMetrics pmetric.ScopeMetrics, resourceMetrics pmetric.ResourceMetrics, cache pcommon.Map) TransformContext {
return 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,
instrumentationScope: instrumentationScope,
resource: resource,
cache: cache,
cache: pcommon.NewMap(),
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 @@ -294,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
Loading

0 comments on commit b241fdd

Please sign in to comment.