-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fix: QS: Log Pipelines: generate correct nil checks for operators referencing fields like attributes["http.status.code"] #4284
Conversation
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
e7cb86a
to
2c5521c
Compare
2c5521c
to
e5ef714
Compare
e5ef714
to
a96a3f2
Compare
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) |
…-fields-with-dots
Since we pass the field paths directly to stanza, keys like Realized the confusion stemming from the PR description. I have changed the description to remove it |
…-fields-with-dots
// 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 |
There was a problem hiding this comment.
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. ;)
There was a problem hiding this comment.
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 ?.
.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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