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

Fix: QS: Log Pipelines: generate correct nil checks for operators referencing fields like attributes["http.status.code"] #4284

Merged

Conversation

raj-k-singh
Copy link
Collaborator

@raj-k-singh raj-k-singh commented Dec 24, 2023

Summary

When using dot separated names for attributes, it should be possible to use paths like attributes["http.status.code"]

This PR includes changes for generating nil checks for field refs with membership op like attributes["http.status.code"]
Before this change, the nil check generation logic did not work for such paths and would generate attributes["http?.status?.code"]

Related Issues / PR's

Fixes: #4208

Affected Areas and Manually Tested Areas

Pipelines. Tested via the test suite and manually e2e

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@github-actions github-actions bot added the bug Something isn't working label Dec 24, 2023
@raj-k-singh raj-k-singh force-pushed the fix/qs-pipeline-support-membership-op-for-fields-with-dots branch 3 times, most recently from e7cb86a to 2c5521c Compare December 26, 2023 10:05
@raj-k-singh raj-k-singh force-pushed the fix/qs-pipeline-support-membership-op-for-fields-with-dots branch from 2c5521c to e5ef714 Compare December 26, 2023 10:11
@raj-k-singh raj-k-singh force-pushed the fix/qs-pipeline-support-membership-op-for-fields-with-dots branch from e5ef714 to a96a3f2 Compare December 26, 2023 11:10
@raj-k-singh raj-k-singh marked this pull request as ready for review December 26, 2023 11:20
@nityanandagohain
Copy link
Member

For a key named "log.file.name" are we supporting attributes["log.file.name"] as well as attributes.log.file.name ? because that would be wrong and might create more confusion

open-telemetry/opentelemetry-collector-contrib#29696 (comment)

@raj-k-singh
Copy link
Collaborator Author

raj-k-singh commented Dec 26, 2023

For a key named "log.file.name" are we supporting attributes["log.file.name"] as well as attributes.log.file.name ? because that would be wrong and might create more confusion

open-telemetry/opentelemetry-collector-contrib#29696 (comment)

Since we pass the field paths directly to stanza, keys like log.file.name will have to be referred to as attributes["log.file.name"] just like stanza and attributes.log.file.name will work only for nested maps

Realized the confusion stemming from the PR description. I have changed the description to remove it

@raj-k-singh raj-k-singh merged commit ec27916 into develop Dec 28, 2023
11 checks passed
@raj-k-singh raj-k-singh deleted the fix/qs-pipeline-support-membership-op-for-fields-with-dots branch December 28, 2023 05:01
// Eg: The field `attributes.test["a.b"].value["c.d"].e` can't be checked using
// the nil check `attributes.test?.["a.b"]?.value?.["c.d"]?.e != nil`
// This needs to be worked around by checking that the target of membership op is not nil first.
// Eg: attributes.test != nil && attributes.test["a.b"]?.value != nil && attributes.test["a.b"].value["c.d"]?.e != nil

Choose a reason for hiding this comment

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

I'm working on adding support for foo?.["bar"] in Expr right now. Will be done in the next version. ;)

Choose a reason for hiding this comment

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

As well as on mode in which all . operations will be set to ?..

Copy link
Collaborator Author

@raj-k-singh raj-k-singh Jan 16, 2024

Choose a reason for hiding this comment

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

Awesome! Thanks @antonmedv that will be super helpful.

We also encountered another potential issue while walking expr ASTs with a visitor for finding all member references.
Expressions like attributes.test1 ?? "hello" were being parsed as MemberNodes while we were expecting the nil coalesce operator to be parsed as a binary node

Choose a reason for hiding this comment

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

Strange. I'm getting BinaryNode:
Screenshot 2024-01-16 at 16 47 22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Log Parsing Pipelines] Using attributes paths like attributes["http.status.code"] doesn't work
3 participants