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

Add a otelcol.processor.transform component #5337

Merged
merged 3 commits into from
Oct 3, 2023

Conversation

ptodev
Copy link
Contributor

@ptodev ptodev commented Sep 29, 2023

PR Description

This PR ports the transform processor to Flow.

Which issue(s) this PR fixes

Fixes #2910

Notes to the Reviewer

When writing flow docs, usually we avoid linking to the documentation of the underlying Otel component so that we don't confuse users with yaml configs which don't apply for the Agent. In this case I had to link to the Otel docs quite a lot. I tried to only link to places where it made sense, and to sections where there isn't any yaml config which could confuse a reader.

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@ptodev ptodev requested review from a team and clayton-cornell as code owners September 29, 2023 18:16
@ptodev ptodev linked an issue Sep 29, 2023 that may be closed by this pull request
@ptodev ptodev mentioned this pull request Sep 29, 2023
@ptodev ptodev force-pushed the 2910-otel-transformprocessor branch from 70a9baf to d437f6d Compare September 29, 2023 18:20
Copy link
Contributor

@clayton-cornell clayton-cornell left a comment

Choose a reason for hiding this comment

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

I'll probably need to go over this one again.

@ptodev ptodev force-pushed the 2910-otel-transformprocessor branch 3 times, most recently from 4bae072 to a1d8caa Compare October 2, 2023 11:00
@ptodev ptodev requested a review from clayton-cornell October 2, 2023 11:00
ptodev and others added 2 commits October 2, 2023 16:17
Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
@ptodev ptodev force-pushed the 2910-otel-transformprocessor branch from a1d8caa to 95cec8f Compare October 2, 2023 15:17

input["error_mode"] = args.ErrorMode

if len(args.TraceStatements) > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this be simplified by removing the len check? Your checking for nil in the convert itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes in the Collector config the behaviour is different even if none of the underlying config is set. E.g. I think this will enable both grpc and http receivers:

receivers:
  otlp:
    protocols:
      grpc:
      http:

But I think this will enable only http:

receivers:
  otlp:
    protocols:
      http:

In the case of this component, I think the behaviour would be the same, but I'd still want to refrain from setting this unless we want this field to be used with its default values.

@ptodev ptodev requested a review from mattdurham October 2, 2023 16:27
Copy link
Contributor

@erikbaranowski erikbaranowski left a comment

Choose a reason for hiding this comment

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

Great work with great examples. I added some nits and a couple reusability suggestions if we anticipate OTTL code/docs being reused by other OTEL components in the future. If not, feel free to disregard those.

@ptodev ptodev requested a review from erikbaranowski October 2, 2023 22:57
Copy link
Contributor

@thampiotr thampiotr left a comment

Choose a reason for hiding this comment

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

LGTM!

LogStatements contextStatementsSlice `river:"log_statements,block,optional"`

// Output configures where to send processed data. Required.
Output *otelcol.ConsumerArguments `river:"output,block"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: If this is required, should we make it a struct instead of a pointer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to keep it consistent between all otelcol components. Not sure why it's a pointer tbh. Maybe to avoid extra copies? @rfratto, do you happen to know why it was originally made this way?

@clayton-cornell clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label Oct 3, 2023
@ptodev ptodev merged commit 04c013d into main Oct 3, 2023
7 checks passed
@ptodev ptodev deleted the 2910-otel-transformprocessor branch October 3, 2023 21:19
tpaschalis pushed a commit to tpaschalis/agent that referenced this pull request Oct 4, 2023
* Add a otelcol.processor.transform component

---------

Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 22, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. type/docs Docs Squad label across all Grafana Labs repos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OTEL: transformprocessor
5 participants