Skip to content

[processor/transform] Fix basic config cache access #39290

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

Conversation

edmocosta
Copy link
Contributor

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:

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.

Link to tracking issue

Fixes #38926

Testing

Unit tests

Documentation

Updated README

…lector-contrib into fix-transform-processor-shared-cache

# Conflicts:
#	processor/transformprocessor/internal/common/logs.go
#	processor/transformprocessor/internal/common/metrics.go
#	processor/transformprocessor/internal/common/processor.go
#	processor/transformprocessor/internal/common/traces.go
@github-actions github-actions bot added processor/transform Transform processor pkg/ottl labels Apr 9, 2025
@edmocosta edmocosta marked this pull request as ready for review April 10, 2025 14:43
@edmocosta edmocosta requested a review from a team as a code owner April 10, 2025 14:43
Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

@edmocosta can you add a new test somewhere, either in the transformprocessor or OTTL's e2e tests, for

log_statements:
      - merge_maps(log.cache, ParseJSON(log.body), "upsert") where IsMatch(log.body, "^\\{")
      - set(log.body, log.cache["log"])

@edmocosta
Copy link
Contributor Author

Hi @TylerHelmuth, thanks for reviewing, I've added a few tests that although are using different statements, does check the exactly same condition.

@TylerHelmuth TylerHelmuth merged commit 68e1d6c into open-telemetry:main Apr 10, 2025
171 checks passed
@github-actions github-actions bot added this to the next release milestone Apr 10, 2025
TylerHelmuth pushed a commit that referenced this pull request Apr 11, 2025
…rom all OTTL contexts (#39338)

<!--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 experimental transform context option `WithCache`
from all OTTL contexts, since they're not used/needed anymore (see
#39290).
akshays-19 pushed a commit to akshays-19/opentelemetry-collector-contrib that referenced this pull request Apr 23, 2025
…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.-->
akshays-19 pushed a commit to akshays-19/opentelemetry-collector-contrib that referenced this pull request Apr 23, 2025
…rom all OTTL contexts (open-telemetry#39338)

<!--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 experimental transform context option `WithCache`
from all OTTL contexts, since they're not used/needed anymore (see
open-telemetry#39290).
Fiery-Fenix pushed a commit to Fiery-Fenix/opentelemetry-collector-contrib that referenced this pull request Apr 24, 2025
…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.-->
Fiery-Fenix pushed a commit to Fiery-Fenix/opentelemetry-collector-contrib that referenced this pull request Apr 24, 2025
…rom all OTTL contexts (open-telemetry#39338)

<!--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 experimental transform context option `WithCache`
from all OTTL contexts, since they're not used/needed anymore (see
open-telemetry#39290).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Valid processor config yields unexpected results
3 participants