Skip to content

Add type safe Getter/Expr and Value#45363

Closed
bogdandrutu wants to merge 1 commit intoopen-telemetry:mainfrom
bogdandrutu:ottlrun
Closed

Add type safe Getter/Expr and Value#45363
bogdandrutu wants to merge 1 commit intoopen-telemetry:mainfrom
bogdandrutu:ottlrun

Conversation

@bogdandrutu
Copy link
Copy Markdown
Member

No description provided.

@bogdandrutu bogdandrutu force-pushed the ottlrun branch 4 times, most recently from 31928fb to 36b7352 Compare January 13, 2026 18:39
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Copy link
Copy Markdown
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a good first step (defining the runtime API), but without example "interpreter" or programs to run this through, it's unclear if it's not just creating the same issues in the current set of evaluators and/or whether it can expand its language feature set going forward.

I'd just a test that creates an expression representing an "OTTL statement" manually from these primitives to get a feel for how it works.

For bonus points (on architecture), I'd use syntax not yet in OTTL, e.g. the proposed for-yield pythonic-generator PR I had from a while back, something like:

set(log.attributes,  [attr for attr in log.attributes if startswith(attr.key, "test")])

You'd need a new Expression type for the loop, something like:

func newLoopExpr[?](collection Expr[?], yield Expr[?], filter Expr[?], itemName string) Expr[?] {
  return newExpr(func(ctx: context.context, tCtx ?) (V, error) {
     // Note I'm ignoring casting for now.  This "expression" should show
     // why, and why the current `tCtx` pattern is detrimental to the future of OTTL.
     list := collection.Eval(ctx, tCtx)
     result := []
     for i, v := range list {
         // Here I need to construct a new context where `Expr` can still
         // find values from the original context, but also *new* things are available.
         //  E.g. I want `atttr` to be the name you use to look up `v` in context.
         listCtx := newListContext(tCtx, itemName, v, i)
         if filter.Eval(ctx, listCtx) {
            result = append(result, yield.Eval(ctx, listCtx))
         }
     }
     return result, nil
  }
}


// Expr is the base struct that represents a function call.
type Expr[K any] interface {
// Disallow implementations outside this package.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just a "keep it private before it's public" thing, or do you want to avoid extension of this type?

type genericExpr[K any, V Value] interface {
Expr[K]

Eval(ctx context.Context, tCtx K) (V, error)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tying an expression to a typed context is one of the big issues with OTTL right now. It means you can't have different "scopes" which prevents things like for-loops, lambdas, etc. from being introduced to the language without huge awkwardness (or straight out shenanigans with casting) in your interpretation engine.

Is this needed for compatibility or can you somehow erase this?

I think you'd be MUCH better off having an "erased" or type-less expression / runtime and putting your type-safe wrappers on the outside of that (at least in the current state of Go generics).

@github-actions
Copy link
Copy Markdown
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions Bot added the Stale label Jan 30, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions Bot closed this Feb 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants