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/telemetryquerylanguage] Add enums to TQL #12337

Merged

Conversation

TylerHelmuth
Copy link
Member

@TylerHelmuth TylerHelmuth commented Jul 12, 2022

Description:
This PR adds Enums to the TQL. This is necessary before the transform processor and be updated to use the package.

The changes are copied from the current Enum implementation in the transformprocessor.

This implementation improves upon the transform processor's version by separating Enums from Paths. This allows us to validate Enums and returns errors if there is no matching Symbol.

Link to tracking Issue:
#11751

Testing:
Updated unit tests

Documentation:
Updated README.

@TylerHelmuth
Copy link
Member Author

/cc @kentquirk

Copy link
Member

@kentquirk kentquirk left a comment

Choose a reason for hiding this comment

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

I think this is generally a good and useful change. I'd like to see the extra tests and to consider whether a more robust error scheme would be handy.

(And selfishly, I'd like to see this rebased after the merge of the and-or branch.)

pkg/telemetryquerylanguage/tql/parser_test.go Outdated Show resolved Hide resolved
pkg/telemetryquerylanguage/tql/functions.go Outdated Show resolved Hide resolved
pkg/telemetryquerylanguage/tql/condition_test.go Outdated Show resolved Hide resolved
@TylerHelmuth
Copy link
Member Author

@kentquirk I added more tests but have not modified the Enum API. Since Enums are being treated as fancy paths, the ParseEnum function doesn't know if the provided Ident is intended to be an Enum or not; it only know that it does or does not match something in the symbol table. If the provided path does not match anything from ParseEnum or PathExpressionParser then ParseQueries will return an error.

I'm open to ideas on how to separate Enum and Path in the grammar, but I don't love the idea of having to add anything to the symbol. Being able to interact with Enums like ... where kind == SPAN_KIND_INTERNAL is very satisfying.

Copy link
Member

@kentquirk kentquirk left a comment

Choose a reason for hiding this comment

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

This makes me happy now. I have no further comments.

@TylerHelmuth
Copy link
Member Author

@open-telemetry/collector-contrib-approvers please review. I'd love to get this PR in so I can complete #12221, all hopefully before Wednesday's release. There are some users who are interested in the new AND and OR where clause capabilities that are in the library but not in the processor.

@TylerHelmuth TylerHelmuth added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jul 15, 2022
@djaglowski djaglowski merged commit b82c5b8 into open-telemetry:main Jul 18, 2022
@TylerHelmuth TylerHelmuth deleted the issue-11751-add-enum-to-tql branch July 18, 2022 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants