Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pkg/ottl] Express context cache as a map[string]any. #26108

Closed
Tracked by #28892
kentquirk opened this issue Aug 25, 2023 · 7 comments
Closed
Tracked by #28892

[pkg/ottl] Express context cache as a map[string]any. #26108

kentquirk opened this issue Aug 25, 2023 · 7 comments
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed pkg/ottl priority:p2 Medium

Comments

@kentquirk
Copy link
Member

Component(s)

pkg/ottl

Is your feature request related to a problem? Please describe.

Separate ticket from #22010

Describe the solution you'd like

We need to update all the cache accessors to be able to handle getting and setting time.Time and time.Duration.

More info to come.

Describe alternatives you've considered

No response

Additional context

@fchikwekwe is intending to work on this. Further comment expected from @TylerHelmuth .

@kentquirk kentquirk added enhancement New feature or request needs triage New item requiring triage labels Aug 25, 2023
@TylerHelmuth TylerHelmuth changed the title [pkg/ottl] Express context cache as a map[string]string. [pkg/ottl] Express context cache as a map[string]any. Aug 25, 2023
@TylerHelmuth TylerHelmuth added pkg/ottl help wanted Extra attention is needed good first issue Good for newcomers priority:p2 Medium and removed needs triage New item requiring triage labels Aug 25, 2023
@TylerHelmuth
Copy link
Member

@evan-bradley with the new Now() function we'll want this functionality so that we can save off a Now() result that could be used for each statement in the list.

This change will impact complex indexing, so we'll have to be careful.

@TylerHelmuth TylerHelmuth removed help wanted Extra attention is needed good first issue Good for newcomers labels Oct 11, 2023
@TylerHelmuth
Copy link
Member

We started working on this and the impact to complex indexing is non-trivial.

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Nov 21, 2023

Related to this problem, I was just shown the scenario

set(resource.attributes["my.environment.2"], Split(resource.attributes["host.name"],"-")[1])

that produces an error:

{"kind": "processor", "name": "transform", "pipeline": "logs", "error": "type, []string, does not support int indexing", "statement": "set(resource.attributes[\"my.environment.2\"], Split(resource.attributes[\"host.name\"],\"-\")[1])"}

Although the language does support indexing the return value of the Converter, it only supports indexing pcommon.Slice and []any, but Split returns a []string. We need to make indexing slices more generic. This might need to be its own issue. Related code:

case k.Int != nil:
switch r := result.(type) {
case pcommon.Slice:
if int(*k.Int) >= r.Len() || int(*k.Int) < 0 {
return nil, fmt.Errorf("index %v out of bounds", *k.Int)
}
result = ottlcommon.GetValue(r.At(int(*k.Int)))
case []any:
if int(*k.Int) >= len(r) || int(*k.Int) < 0 {
return nil, fmt.Errorf("index %v out of bounds", *k.Int)
}
result = r[*k.Int]
default:
return nil, fmt.Errorf("type, %T, does not support int indexing", result)
}

@evan-bradley
Copy link
Contributor

I think this is a separate issue, but I agree that we should be able to index any slice type that is supported in OTTL.

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Jan 25, 2024

I've started thinking about this problem again and I keep getting stuck on how much we lose my making cache a map[string]any instead of a pcommon.Map. For most scenarios it is very beneficial to have a pcommon.Map since we are already working with the data in pdata format. To switch to a map[string]any means translating back and forth, which should not be necessary most of the time.

I've come up with a couple alternative solutions I'd like to discuss.

  1. Leave cache as it is and add a new accessor for storing non-pcommon.Value types such as time.Time and time.Duration. This has the benefit of performance, but I think there could be confusion with which accessor to use, cache or this other one.

  2. Continue using cache as the path accessor, but for non-pcommon.Value types, store the value in a different map behind the scenes. This add increased complexity in the Context implementation of the accessor, but hides those details from the user. I think there is probably a way while setting a value to record that this key is not a pdata value and then save the value off in some other map[string]any. Then when getting we'd always check if the key is for a non-pcommon.Value type, which is at least a constant-time check.

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Mar 25, 2024

Thinking about this issue some more:

If the primary goal is to be able to store time.Time in the cache, then one solution is to convert time.Time to int64 using UnixSeconds and then convert back to a time.Time using Unix.

I believe all other types can be expressed in the pcommon.Map via a pcommon.Value. If this is true then we could potential close this issue until more non-pdata types come up.

@TylerHelmuth
Copy link
Member

With the release of the Unix function in v0.98.0 I am going to close this issue for now. I believe there are sufficient solutions for storing a timestamp if needed and the efficiency we're gaining from using a pcommon.Map is one of the reasons we chose to allow OTTL to closely tie itself to pdata.

If this issue comes up again we can reopen this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed pkg/ottl priority:p2 Medium
Projects
None yet
Development

No branches or pull requests

3 participants