Feedback to improve tql package

[bogdandrutu]

• ExprFunc, maybe Gettter.Get, Setter.Set should all return errors I believe. I am certain for ExprFunc and Setter (for invalid type) that we should do it, not 100% for Get, but it is not bad to do.
• Pass ctx context.Contex to all these funcs? Maybe part of the TransformContext?
• Look into transform the TransformContext interface into a struct to avoid unnecessary allocations. I think right now we have an allocation for each Span/LogRecord/DataPoint.
• tql.Literal can be private based on the current usages. Look into all the types and avoid exposing public any unnecessary type.
• Better explain the types, none of the public types have public comments.
• I don't understand why do we need an Enum type and EnumParser. Can we have the GetSetter for enum deal with conversion to/from strings if needed and avoid exposing this concept?
• Hide internal implementation of the Query. Just make it a struct with func (q *Query) Evaluate(ctx TransformContext) error. This will allow us to move all types form parser.go as private (if possible) or in a internal package.

[TylerHelmuth]

I don't understand why do we need an Enum type and EnumParser. Can we have the GetSetter for enum deal with conversion to/from strings if needed and avoid exposing this concept?

See #10349 for some Enums decisions. The current implementation allows the grammar to convert EnumSymbols to int64 during startup instead of during the hot path. Not saying the current implementation is the only way to achieve that, but I think it important that we handle Enums during parsing instead of during runtime.

Also important that the Context is the source of truth for Enums and not the grammar.

[TylerHelmuth]

Hide internal implementation of the Query. Just make it a struct with func (q *Query) Evaluate(ctx TransformContext) error. This will allow us to move all types form parser.go as private (if possible) or in a internal package.

I plan to do something like this for #13545. Waiting on #13544 before moving forward with that refactor.

[TylerHelmuth]

Pass ctx context.Contex to all these funcs? Maybe part of the TransformContext?

We don't have a use case for this today but I'm curious if you've got one in mind.

[bogdandrutu]

We don't have a use case for this today but I'm curious if you've got one in mind.

Things can be retrieved from the context (maybe?)?

[TylerHelmuth]

Look into transform the TransformContext interface into a struct to avoid unnecessary allocations.

We could do this if we stopped having a TransformContext per signal and had one struct with all the functions (GetItem, GetResource, GetInstrumentationScope, GetMetric, GetMetrics) to access all the data in the context. But we would be severely reducing the usability of the package anywhere outside the collector. So much so that if we got rid of the TransformContext interface we might want to move the package from pkg into internal.

I like the concept that the TQL is as generic as possible, though, so I'd like to keep it in pkg.

[github-actions]

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

• pkg/ottl: @TylerHelmuth @kentquirk @bogdandrutu @evan-bradley

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[github-actions]

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

• pkg/ottl: @TylerHelmuth @kentquirk @bogdandrutu @evan-bradley

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[github-actions]

This issue has been closed as inactive because it has been stale for 120 days with no activity.