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.

Sign up for GitHub

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

Jump to bottom

[receivercreator] add support for logs and traces #19641

Merged
merged 1 commit into from
May 17, 2023
Merged

[receivercreator] add support for logs and traces #19641

merged 1 commit into from
May 17, 2023

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Mar 14, 2023 •
edited
Loading

Description:
Add support for logs and traces with receivercreator component

Link to tracking Issue:
Fixes #19205

Testing:
Unit tests

Documentation:
Added to README.

@atoulme atoulme requested review from a team and codeboten March 14, 2023 05:29
@atoulme atoulme force-pushed the add_logs_to_receivercreator branch from 2b9e10f to 115ba9d Compare March 14, 2023 05:50
@rmfitzpatrick
Copy link
Contributor

Having the receiver creator factory support logs is required, but you also need to connect to the instantiated receiver instances:

opentelemetry-collector-contrib/receiver/receivercreator/runner.go

Line 146 in 2f8e6f7

return factory.CreateMetricsReceiver(context.Background(), runParams, cfg, nextConsumer)

@atoulme
Copy link
Contributor Author

atoulme commented Mar 18, 2023

Thank you Ryan, I'll look into it.

@atoulme atoulme force-pushed the add_logs_to_receivercreator branch 2 times, most recently from ed27967 to 1cc5772 Compare March 22, 2023 15:04
@atoulme
Copy link
Contributor Author

atoulme commented Mar 23, 2023

More thinking required for this PR - @rmfitzpatrick will pick this up as I am at the end of my wits.

@atoulme atoulme added the on hold This is blocked by another PR/issue label Mar 23, 2023
@rmfitzpatrick
Copy link
Contributor

I've updated this branch to also cover #19206 since the refactoring bit made it a trivial functional enhancement (but a bit more verbose on test changes).

@rmfitzpatrick rmfitzpatrick force-pushed the add_logs_to_receivercreator branch 3 times, most recently from 182a564 to c64b719 Compare April 7, 2023 20:06
atoulme
atoulme commented Apr 7, 2023
View reviewed changes
Copy link
Contributor Author

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

I would approve but I opened this PR :)

@atoulme atoulme removed the on hold This is blocked by another PR/issue label Apr 7, 2023
@rmfitzpatrick rmfitzpatrick force-pushed the add_logs_to_receivercreator branch 4 times, most recently from acb26bb to f519c23 Compare April 10, 2023 12:33
pmcollins
pmcollins approved these changes Apr 11, 2023
View reviewed changes
Copy link
Member

@pmcollins pmcollins left a comment

Choose a reason for hiding this comment

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

Just a couple of nits.

@rmfitzpatrick rmfitzpatrick force-pushed the add_logs_to_receivercreator branch 3 times, most recently from 8c29e21 to f2edbcf Compare May 3, 2023 16:43
@rmfitzpatrick
Copy link
Contributor

@codeboten mind taking another look when able?

codeboten
codeboten reviewed May 5, 2023
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

This looks good, will need another rebase with some fixes, see my suggestions. Would be happy to get this merged once this is done

receiver/receivercreator/metadata.yaml Outdated Show resolved Hide resolved
receiver/receivercreator/factory.go Outdated Show resolved Hide resolved
codeboten
codeboten approved these changes May 5, 2023
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Just one more non-blocking comment. Thanks for addressing my other comments

receiver/receivercreator/README.md Outdated Show resolved Hide resolved
@atoulme atoulme force-pushed the add_logs_to_receivercreator branch from 999187e to 8b33fd2 Compare May 16, 2023 22:30
@open-telemetry open-telemetry deleted a comment from runforesight bot May 16, 2023
@atoulme atoulme force-pushed the add_logs_to_receivercreator branch from 8b33fd2 to 723cbaf Compare May 17, 2023 00:19
@atoulme atoulme changed the title [receivercreator] add support for logs [receivercreator] add support for logs and traces May 17, 2023
@dmitryax
Copy link
Member

@atoulme can you please rebase?

@atoulme atoulme force-pushed the add_logs_to_receivercreator branch from 723cbaf to eb1988a Compare May 17, 2023 04:13
@dmitryax dmitryax merged commit 1b921c8 into open-telemetry:main May 17, 2023
@github-actions github-actions bot added this to the next release milestone May 17, 2023
theletterf
theletterf reviewed Jun 1, 2023
View reviewed changes
receiver/receivercreator/README.md
Comment on lines +24 to +26
If you use the receiver creator in multiple pipelines of differing telemetry types,
but a given dynamically instantiated receiver doesn't support one of the pipeline's type,
it will effectively lead to a logged no-op that won't cause a collector service failure.
Copy link
Member

Choose a reason for hiding this comment

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

@dmitryax Could you provide an example of this?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If you use the receiver creator in multiple pipelines of differing telemetry types,
but a given dynamically instantiated receiver doesn't support one of the pipeline's type,
it will effectively lead to a logged no-op that won't cause a collector service failure.
> Note: When using receiver creator in multiple pipelines with different telemetry types, the Collector might crash if one of the receivers doesn't support a pipeline type. Make sure the instantiated receivers can support the pipeline type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rmfitzpatrick is probably better placed to comment, but I think you misunderstood what is documented there. Anyway, happy to clarify in a separate issue: #23014

@atoulme atoulme mentioned this pull request Jun 1, 2023
Closed
@rmfitzpatrick rmfitzpatrick mentioned this pull request Jul 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theletterf theletterf theletterf left review comments

@rmfitzpatrick rmfitzpatrick rmfitzpatrick left review comments

@pmcollins pmcollins pmcollins approved these changes

@codeboten codeboten codeboten approved these changes

@dmitryax dmitryax dmitryax approved these changes

Assignees

@djaglowski djaglowski

Labels
receiver/receivercreator
Projects
None yet
Milestone
v0.78.0
Development

Successfully merging this pull request may close these issues.

[receiver/receivercreator] Add support for logs
7 participants
@atoulme @rmfitzpatrick @dmitryax @pmcollins @codeboten @theletterf @djaglowski