Improve error when "null" is used instead of "nil" in OTTL: invalid path expression [{null []}]

[ringerc]

Component(s)

pkg/ottl

Describe the issue you're reporting

When an OTTL expression uses the term null instead of the intended nil, the resulting error is not especially helpful, e.g.:

    Error: processors::transform/afterdiscovery: unable to parse OTTL statement "set(attributes[\"keep\"], resource.attributes[\"keep\"]) where resource.attributes[\"keep\"] != null": invalid path expression [{null []}]

The user should've written != nil but the error could definitely help the user out.

Same would make sense to do for NULL, NUL and nul.

[github-actions]

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.

[evan-bradley]

I agree that error isn't the best, I would be okay with catching null-variations and printing a message that nudges users in the right direction. At a minimum we should be printing a clearer error message for invalid paths to help explain where the user should look to fix the error.

[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.

[TylerHelmuth]

It would be a weird case, but technically a context someone creates could use null as a valid path name, so I don't think we want to do any special handling of null as it will exclude that keyword as a valid path. Claiming keywords like nil in the language seems ok, but claiming keywords to exclude them from use feels less good.

We can improve the error message tho - I've opened a PR.

[evan-bradley]

Claiming keywords like nil in the language seems ok, but claiming keywords to exclude them from use feels less good.

In my opinion, the language should own concepts like null values. Having a keyword called nil and a path called null feels like we're entering the realm of null and undefined in JavaScript where the terms start to lose their meaning. Take the following statement for example, which I think would be bad UX if a context ever introduced it:

set(attributes["test"], "value") where null == nil

All that said, I think even just improving the error message is a win here.

[TylerHelmuth]

I agree that set(attributes["test"], "value") where null == nil is weird, and we'd never introduce null as a path in pkg/ottl/contexts, but I'm not convinced yet we should limit a user's ability to use null as a path in their own custom context should they choose to.

[evan-bradley]

I agree that we should keep OTTL's keywords to a minimum (I would count even catching null as having it as a keyword), but I think the number of cases where the compiler can help new users figure out which term represents null values in OTTL will be dramatically higher than the number of cases where a custom context will want to use null as a path. There's also a handful of other terms that could be used that aren't as common as null value keywords: "empty", "void", "nothing", etc.

I think the best argument I could give is that if we add it before beta, it won't be a breaking change to later remove the excluded synonyms if someone says they want null as a path in their context. Statements in all contexts would still fail parsing, but would just have a different error messages.

[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.