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 filereload receiver #15660

Closed
wants to merge 2 commits into from
Closed

Conversation

newly12
Copy link
Contributor

@newly12 newly12 commented Oct 26, 2022

Description:

Add filereload receiver to support dynamically start/stop partial pipelines(receiver, processor) from configs.

Link to tracking Issue:
#15714

Testing:
UT.

Documentation:
Please check README doc.

@newly12 newly12 requested review from a team and codeboten October 26, 2022 04:59
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

This needs an issue and sponsor in accordance to the contributing guidelines for new components. Thanks!

@newly12
Copy link
Contributor Author

newly12 commented Oct 31, 2022

Had filed issue. I can fix accordingly per the guidelines suggested if the idea looks good generally. Feel free to let me know if any question.

@tigrannajaryan
Copy link
Member

tigrannajaryan commented Nov 4, 2022

This looks very interesting, however I think functionality like this may be more useful if broken down into 3 composable parts:

  • Watching of configuration in external file(s). This should work for any part of the config, instead of being a feature of a particular receiver. We already have a mechanism for this, but it is not implemented by fileprovider config provider yet. We already allow including external config files, but we don't watch for changes.
  • Pipelines that can be connected. We have an open issue for this Signal Translation Proprosal opentelemetry-collector#2336
  • A receiver that can create other receivers. Need to figure out how it intersects with the existing receivercreator.

These bits can then be composed to result in the functionality that this PR suggests, but they can also be composed in other interesting ways.

@open-telemetry/collector-approvers @open-telemetry/collector-contrib-approvers thoughts?

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Nov 19, 2022
@mx-psi mx-psi removed the Stale label Nov 21, 2022
@newly12
Copy link
Contributor Author

newly12 commented Nov 22, 2022

Thanks for the comment, I will take a look at confmap and see how it can be leveraged in this receiver and get back soon.

@newly12
Copy link
Contributor Author

newly12 commented Nov 23, 2022

Watching of configuration in external file(s). This should work for any part of the config, instead of being a feature of a particular receiver. We already have a mechanism for this, but it is not implemented by fileprovider config provider yet. We open-telemetry/opentelemetry-collector#6276 including external config files, but we don't watch for changes.

open-telemetry/opentelemetry-collector#5945 for watch implementation in fileprovider, however it looks like the current mechanism only allows external config for a component(receiver, processor?, exporter), and on config changes entire service will be restarted(by reloadConfiguration), which is expensive if there is no change for some components, say exporters are not updated but only receiver configs are updated.

Pipelines that can be connected. We have an open issue for this open-telemetry/opentelemetry-collector#2336

open-telemetry/opentelemetry-collector#6372 for connector implementation.

A receiver that can create other receivers. Need to figure out how it intersects with the existing receivercreator.
I think either a receiver that creates other receivers or a mechanism starts/stops pipelines dynamically.

In fact the need is to spawn not only receivers but also processors, given usually different receivers(data source) require different processing logical, so maybe a mechanism to spawn receivers and processors(it will nice to be able to spawn exporter as well), it sounds like mechanism/ability to spawn pipelines dynamically will be needed.

@tigrannajaryan
Copy link
Member

tigrannajaryan commented Nov 23, 2022

on config changes entire service will be restarted(by reloadConfiguration), which is expensive if there is no change for some components, say exporters are not updated but only receiver configs are updated.

This is correct and one of the things we discussed in the past and wanted to do is to avoid restarting all components on config changes and only restart components that changed (plus components that are earlier in the pipeline since the chaining model requires it). This would be another great addition to the Collector.

@jpkrohling
Copy link
Member

@newly12, as @tigrannajaryan pointed out, this is part of a bigger discussion. I encourage you to join the collector SIG calls if you haven't yet (I missed the last few ones myself). I'm closing this PR, but this should not discourage you from contributing features that will lead to the outcome you wanted when you proposed this.

@jpkrohling jpkrohling closed this Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants