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

FluentBit Subprocess Extension #1381

Merged
merged 1 commit into from
Jul 28, 2020
Merged

FluentBit Subprocess Extension #1381

merged 1 commit into from
Jul 28, 2020

Conversation

keitwb
Copy link
Contributor

@keitwb keitwb commented Jul 16, 2020

This extension runs FluentBit as a subprocess and does some standard
configuration to make it talk to a fluentforward receiver running in the
same collector.

You still have to pass through the FluentBit config text but it makes
running FluentBit with the collector a bit easier.

Only the second commit is relevant for the extension.

extension/fluentbitextension/README.md Show resolved Hide resolved
extension/fluentbitextension/README.md Show resolved Hide resolved
extension/fluentbitextension/README.md Outdated Show resolved Hide resolved
configmodels.ExtensionSettings `mapstructure:",squash"`

// The TCP `host:port` to which the subprocess should send log entries.
// One of this or `unix_socket_path` are required.
Copy link
Member

Choose a reason for hiding this comment

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

unix_socket_path does not seem to exist in this config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a leftover from my initial draft. The FluentBit output plugin currently doesn't support unix sockets so there is no point in the extension supporting it at this point.

extension/fluentbitextension/config.go Show resolved Hide resolved
extension/fluentbitextension/config.go Show resolved Hide resolved
// procWait is guaranteed to be sent exactly one message per successful process start
procWait := make(chan error)

// A state machine makes the management easier to understand and account
Copy link
Member

Choose a reason for hiding this comment

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

👍

extension/fluentbitextension/process.go Outdated Show resolved Hide resolved
extension/fluentbitextension/process.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 17, 2020

Codecov Report

Merging #1381 into master will increase coverage by 0.03%.
The diff coverage is 93.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1381      +/-   ##
==========================================
+ Coverage   90.83%   90.86%   +0.03%     
==========================================
  Files         234      237       +3     
  Lines       16472    16593     +121     
==========================================
+ Hits        14962    15077     +115     
- Misses       1082     1086       +4     
- Partials      428      430       +2     
Impacted Files Coverage Δ
extension/fluentbitextension/process.go 92.45% <92.45%> (ø)
extension/fluentbitextension/factory.go 100.00% <100.00%> (ø)
extension/fluentbitextension/process_linux.go 100.00% <100.00%> (ø)
service/defaultcomponents/defaults.go 84.31% <100.00%> (+0.31%) ⬆️
translator/internaldata/resource_to_oc.go 86.04% <0.00%> (+2.32%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39521ed...071edde. Read the comment docs.

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM.

@tigrannajaryan
Copy link
Member

Please rebase from master and fix the conflict.

@tigrannajaryan
Copy link
Member

check-links failed because apparently zipkin.io domain is expired :-(

@keitwb
Copy link
Contributor Author

keitwb commented Jul 22, 2020

The four lines that aren't hit in coverage are either from underlying OS errors or very difficult to induce race conditions in the management of the subprocess.

@tigrannajaryan
Copy link
Member

The four lines that aren't hit in coverage are either from underlying OS errors or very difficult to induce race conditions in the management of the subprocess.

Sounds good to me. @bogdandrutu are you OK with the coverage?

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Can you explain in a README why is this an extension and not a receiver, I think I miss this.

extension/fluentbitextension/config_test.go Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member

@keitwb Please fix the build and conflicts.

@keitwb keitwb requested a review from nilebox as a code owner July 24, 2020 20:30
Copy link
Contributor Author

@keitwb keitwb left a comment

Choose a reason for hiding this comment

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

Can you explain in a README why is this an extension and not a receiver, I think I miss this.

The difference between the receiver and extension for fluent is made a bit clearer now.

@tigrannajaryan
Copy link
Member

Can you explain in a README why is this an extension and not a receiver, I think I miss this.

The difference between the receiver and extension for fluent is made a bit clearer now.

@bogdandrutu is it OK with you now?

extension/fluentbitextension/README.md Outdated Show resolved Hide resolved
"syscall"
)

func applyOSSpecificCmdModifications(cmd *exec.Cmd) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this not work on darwin or any other os?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, just Linux for the initial version. We can add support for Mac/Windows later if this proves to be a desirable approach to having the collector manage fluentbit.

This extension runs FluentBit as a subprocess and does some standard
configuration to make it talk to a fluentforward receiver running in the
same collector.

You still have to pass through the FluentBit config text but it makes
running FluentBit with the collector a bit easier.
@tigrannajaryan
Copy link
Member

@bogdandrutu I am good with this PR, reassigning to you to give the final approval.

@bogdandrutu bogdandrutu merged commit 72e41e3 into open-telemetry:master Jul 28, 2020
@keitwb keitwb deleted the fluent-subproc branch July 30, 2020 13:49
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
* Let SynchronizedMove(nil) reset and discard

* Add common test for SynchronizedMove(nil)

* End-to-end test for the Processor and SumObserver

* Implement SynchronizedMove(nil) six ways

* Lint

* Changelog

* Test no reset for wrong aggregator type; Fix four Aggregators

* Cleanup

* imports

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
…y#1381)

Bumps [boto3](https://github.com/boto/boto3) from 1.21.22 to 1.21.23.
- [Release notes](https://github.com/boto/boto3/releases)
- [Changelog](https://github.com/boto/boto3/blob/develop/CHANGELOG.rst)
- [Commits](boto/boto3@1.21.22...1.21.23)

---
updated-dependencies:
- dependency-name: boto3
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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