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

[processor/transform] more readme refactoring #37909

Merged

Conversation

TylerHelmuth
Copy link
Member

Description

With the new context inference feature the user needs to know less about the concept of an OTTL Context. This refactor restructures the README some more to try really hard not to talk about OTTL context and instead talk about OTTL Paths, which is the concrete thing the user needs to know to write an OTTL statement.

Link to tracking issue

#29017

Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

Overall I really like this. I'd be fine moving forward without any of the larger changes I suggested, we can always iterate later.

processor/transformprocessor/README.md Outdated Show resolved Hide resolved
processor/transformprocessor/README.md Outdated Show resolved Hide resolved
processor/transformprocessor/README.md Outdated Show resolved Hide resolved
processor/transformprocessor/README.md Outdated Show resolved Hide resolved
processor/transformprocessor/README.md Outdated Show resolved Hide resolved
processor/transformprocessor/README.md Outdated Show resolved Hide resolved
processor/transformprocessor/README.md Outdated Show resolved Hide resolved
processor/transformprocessor/README.md Outdated Show resolved Hide resolved
processor/transformprocessor/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@edmocosta edmocosta left a comment

Choose a reason for hiding this comment

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

It looks great! I liked the new format and extra OTTL section.

processor/transformprocessor/README.md Outdated Show resolved Hide resolved
processor/transformprocessor/README.md Outdated Show resolved Hide resolved
processor/transformprocessor/README.md Outdated Show resolved Hide resolved
processor/transformprocessor/README.md Outdated Show resolved Hide resolved
processor/transformprocessor/README.md Outdated Show resolved Hide resolved
@crobert-1 crobert-1 added the documentation Improvements or additions to documentation label Feb 13, 2025
@edmocosta
Copy link
Contributor

Would we be able to get away without introducing any new concepts and just say that each item in the X_statements list is:
"Basic config": A statement
"Advanced config": A nested config object within the list that contains a list of statements and options for configuring their > execution.

It's getting great! I liked @evan-bradley suggestion, but reading the docs it's still feel we're introducing new concepts, but now with different names "Flat configuration" -> "Basic Config", and "Structured configuration" -> "Advanced Configuration". I'm wondering if we should focus on the statements configuration part instead, so it wouldn't feel those styles are completely different configuration formats, for example: TylerHelmuth/opentelemetry-collector-contrib@transform-continue-readme-refactor...edmocosta:opentelemetry-collector-contrib:patch-1

@evan-bradley
Copy link
Contributor

I'm wondering if we should focus on the statements configuration part instead, so it wouldn't feel those styles are completely different configuration formats

I was hoping that at least with terms like "basic" or "advanced" configuration users would have some pre-existing idea of what those mean, but I agree we should try to make them feel like the same thing.

I like what you're going for, but I think I would tweak it just slightly and say something like this:

The Transform Processor's main config section is a list of things for the processor to execute. The list can contain:

  1. A string, which is a statement to execute. Most users should opt for this, at least at first.
  2. A subobject, which allows users advanced configuration options applied to a sub-list of statements, including conditions, a specific error mode, etc. (note all statements here must work on the same type of data).

I think if we frame it as "diving deeper" instead of making a choice between two styles they will feel like a single thing.

Copy link
Contributor

@edmocosta edmocosta left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for improving the docs!

Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

Looks good to me. This is a big improvement for the docs.

processor/transformprocessor/README.md Outdated Show resolved Hide resolved
@TylerHelmuth TylerHelmuth added the ready to merge Code review completed; ready to merge by maintainers label Feb 14, 2025
@TylerHelmuth TylerHelmuth merged commit 1c37438 into open-telemetry:main Feb 14, 2025
152 of 153 checks passed
@github-actions github-actions bot added this to the next release milestone Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation pkg/ottl processor/transform Transform processor ready to merge Code review completed; ready to merge by maintainers Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants