-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Valid processor config yields unexpected results #38926
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
Comments
Pinging code owners:
See Adding Labels via Comments if you do not have permissions to add labels yourself. |
Hi @JelleSmet-TomTom, thank you for reporting, that's indeed an issue with the new basic configuration style and the |
Hey, I would like to look at this issue. |
Hi @odubajDT, I'm sorry, I forget to assign this issue to myself. I've been working on a solution already and the draft still needs to be discussed and validated with maintainers. Hope you don't mind I take this one. Thanks! |
Sure no issues, thanks for the info! |
…9290) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description This PR removes the shared cache logic we introduced to support Basic Config statements, and changes the approach to group all Basic config statements together into the same sharing-cache `common.ContextStatements`, embracing the limitation of not being able to infer the context depending on the user's statements (similar to the advanced config). To help with that limitation, the context inferrer validation was improved, and it now returns specific errors that better describe why it couldn't infer a valid context. This PR also introduces a **breaking change**, that consists in forcing users to use either basic or advanced configuration, for example, mixed configurations like the following won't be valid anymore, and it will require users to use only one style: ```yaml log_statements: - set(resource.attributes["foo"], "foo") - statements: - set(resource.attributes["bar"], "bar") ``` --- Another PR will be open for removing the `WithCache` option from all OTTL contexts. <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes open-telemetry#38926 <!--Describe what testing was performed and which tests were added.--> #### Testing Unit tests <!--Describe the documentation added.--> #### Documentation Updated README <!--Please delete paragraphs that you did not use before submitting.-->
…9290) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description This PR removes the shared cache logic we introduced to support Basic Config statements, and changes the approach to group all Basic config statements together into the same sharing-cache `common.ContextStatements`, embracing the limitation of not being able to infer the context depending on the user's statements (similar to the advanced config). To help with that limitation, the context inferrer validation was improved, and it now returns specific errors that better describe why it couldn't infer a valid context. This PR also introduces a **breaking change**, that consists in forcing users to use either basic or advanced configuration, for example, mixed configurations like the following won't be valid anymore, and it will require users to use only one style: ```yaml log_statements: - set(resource.attributes["foo"], "foo") - statements: - set(resource.attributes["bar"], "bar") ``` --- Another PR will be open for removing the `WithCache` option from all OTTL contexts. <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes open-telemetry#38926 <!--Describe what testing was performed and which tests were added.--> #### Testing Unit tests <!--Describe the documentation added.--> #### Documentation Updated README <!--Please delete paragraphs that you did not use before submitting.-->
Hello @JelleSmet-TomTom, Have you tried the new bug-fixed version of Open Telemetry? Best regards, |
Hi @Damian92666, could you please provide more details on that? I would be glad to take a look at it. |
Hi @edmocosta, Thank you for provided examples. You are right. This is working as expected. I have a different issue, with resource attributes, but I think we have an answer here: To avoid mixing labels, we should only use resource-related labels (like hostname, env_name...). Thanks, |
Thanks for confirming @Damian92666! BTW, if you need to set resource attributes using different log record data, you can enable the transform.flatten.logs feature gate, and set |
Thank you @edmocosta! It works! This is what I wanted to see.. :) |
Uh oh!
There was an error while loading. Please reload this page.
Component(s)
processor/transform
What happened?
Description
The transform documentation mentions in the Basic Config section that
log_statements
is a list consisting out of the individual expressions as strings.This format, at least in my particular test, yields unexpected (and therefor wrong) results. It seems that for every line of the input file, a log is generated but each log has the content of the line of the
input.log
fileWhen the same configuration is converted to the advanced configuration style results are correct.
It is unclear where the difference comes from.
Steps to Reproduce
create file
input.log
with following content:Apply the below mentioned configuration.
working processor config
Replace the
processors
section of the broken config with this and the result is what is expected.Expected Result
output.log
contains:Actual Result
output.log
contains:Collector version
v0.120.0 and latest
Environment information
Environment
official docker images
OpenTelemetry Collector configuration
(causes the faulty results)
Log output
Additional context
No response
The text was updated successfully, but these errors were encountered: