-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat: Add Blog Post about unroll in the contrib distribution
#8097
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
base: main
Are you sure you want to change the base?
Conversation
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.
thank you for committing this PR, some initial feedback.
|
@open-telemetry/collector-approvers PTAL |
Co-authored-by: Severin Neumann <severin.neumann@altmuehlnet.de>
Co-authored-by: Severin Neumann <severin.neumann@altmuehlnet.de>
…o blog/unroll-processor
…etry.io into blog/unroll-processor
|
✅ |
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.
took another closer look, overall LGTM, review is more around style and some copy-editing.
| ```yaml | ||
| receivers: ... | ||
|
|
||
| processors: | ||
| transform: | ||
| error_mode: ignore | ||
| log_statements: | ||
| - context: log | ||
| statements: | ||
| - set(body, Split(body, "\"},")) where true | ||
| unroll: {} | ||
| exporters: ... | ||
|
|
||
| services: | ||
| pipelines: | ||
| logs: | ||
| receivers: [...] | ||
| processors: [transform, unroll] | ||
| exporters: [...] | ||
| ``` |
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.
this looks like a duplication of the YAML section before?
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.
Ah while it is a similar snippet its mostly pertaining to the example I'm trying to highlight for the use case. I'd be happy to think of ways to avoid the yaml dump though. Perhaps the only section needed here is the processors section? WDYT?
| ```yaml | |
| receivers: ... | |
| processors: | |
| transform: | |
| error_mode: ignore | |
| log_statements: | |
| - context: log | |
| statements: | |
| - set(body, Split(body, "\"},")) where true | |
| unroll: {} | |
| exporters: ... | |
| services: | |
| pipelines: | |
| logs: | |
| receivers: [...] | |
| processors: [transform, unroll] | |
| exporters: [...] | |
| ``` | |
| ```yaml | |
| processors: | |
| transform: | |
| error_mode: ignore | |
| log_statements: | |
| - context: log | |
| statements: | |
| - set(body, Split(body, "\"},")) where true | |
| unroll: {} |
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.
the only difference I see between this block and the one above is the where true but I am not sure if I understand from the context of the surrounding text what differences this makes:
- set(body, Split(body, "\"},"))
vs
- set(body, Split(body, "\"},")) where true
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.
Yeah the context was more to highlight just the snippets of processors vs the whole otel configuration. I've gone ahead an consolidated to just one YAML snippet!
Where true is similar to a SQL 1=1 where it just makes it easer to add where clauses and not needed. I think with the consolidated config it is an overall improvement!
Co-authored-by: Severin Neumann <severin.neumann@altmuehlnet.de>
Adds a new blog post around our work in upstreaming
unrollFixes: #8039