Subprocess manager and its tests for the prometheus_exec Receiver #499

Nicolas-MacBeth · 2020-07-23T19:55:25Z

Please look at #453 for the full PR. Since that PR was too big, I'm breaking it up into 2 different PRs, starting with this one. The entire feature/Receiver was written so please refer back to it for better context if need be! This is the subprocess manager part of the receiver, an sub-package of the receiver!

Test coverage is 89% since the lines that are not covered are OS-level error returns that are hard to replicate in testing.

codecov · 2020-07-23T20:05:08Z

Codecov Report

Merging #499 into master will decrease coverage by 0.01%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##           master     #499      +/-   ##
==========================================
- Coverage   86.03%   86.01%   -0.02%     
==========================================
  Files         187      188       +1     
  Lines       10142    10193      +51     
==========================================
+ Hits         8726     8768      +42     
- Misses       1093     1098       +5     
- Partials      323      327       +4

Flag	Coverage Δ
#integration	`71.09% <ø> (ø)`
#unit	`85.84% <82.35%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...rometheusexecreceiver/subprocessmanager/manager.go	`82.35% <82.35%> (ø)`

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 2be82e8...d83f304. Read the comment docs.

james-bebbington

Overall this PR looks good. Mostly only minor suggestions.

However, I would recommend starting with a PR that adds the Receiver skeleton and a shell for the manager that just does nothing rather than starting with this manager. You will likely need an interface + mock implementation of the manager anyway in order to write tests and get your test coverage up.

It's usually better to start from client code, and build up to an implementation that does something interesting. Otherwise you're building writing code and expecting / hoping it will meet the needs of the client that doesn't exist yet (even though in this case you know what's coming, but it will still cause issues if any refactoring is needed), as well as requiring reviewers to imagine how this will be used.

In terms of getting test coverage up in general and ensuring you have tests to cover typical error conditions, the usual way of doing this in Go is to write an interface with the functions defined in the external package (e.g. StdoutPipe(), Start(), etc). Then you can mock the interface for testing and inject your mocks. You will need to refactor your code somewhat to make this possible (e.g. factor out the code that creates the Command object) but you should be able to test nearly everything.

james-bebbington · 2020-07-24T07:08:44Z

receiver/prometheusexec/subprocessmanager/manager.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package subprocessmanager


Does subprocessmanager need to be in a separate package? It's not that large; I would recommend just putting it directly in the prometheusexecreceiver package, unless you have a good reason why you want it to be a separate package such that people can consume it as a standalone package.

I had discussions in the past with Quentin where we thought it would be a good idea to leave it as a separate package other can consume! It keeps track of runtime and has methods to compute exponential backoff, pipe the output of the subprocesses and format the environment variables!

receiver/prometheusexec/subprocessmanager/config/config.go

receiver/prometheusexec/subprocessmanager/manager.go

receiver/prometheusexec/Makefile

receiver/prometheusexec/subprocessmanager/manager.go

james-bebbington · 2020-07-24T07:34:44Z

receiver/prometheusexec/subprocessmanager/manager.go

+	return envSlice
+}
+
+// GetDelay will compute the exponential backoff for a given process according to its crash count and time alive


Suggested change

// GetDelay will compute the exponential backoff for a given process according to its crash count and time alive

// GetDelay will compute the delay for a given process according to its crash count and time alive using an exponential backoff algorithm

Also consider if it would make sense to use https://github.com/cenkalti/backoff instead of writing this logic yourself. I think that package is used in a few other places in the Collector already

Changed the wording! I prefer using my own logic since I have more power with the use of computing the backoff with crashCount as well as runtime (elapsed). Since I'm also managing the receiver associated to the subprocess, there's lots of different conditions that define whether a process is "healthy" or not!

receiver/prometheusexec/subprocessmanager/manager_test.go

…omments

go.sum

Nicolas-MacBeth · 2020-07-27T15:57:39Z

I spoke with James offline in regards to his summary comment, and I'll try moving the PR this way (only the subprocess manager part of the receiver) for two main reasons: the entire feature/PR is done (the rest of the code is ready to go and will be made into a PR as soon as this is merged!) and it is referenced for context in this PR's description.

This happened because I made the entire feature and then retroactively needed to break the PR into smaller chunks. In the future I'll be more mindful about how to break up my work beforehand. We'll discuss further tonight as well.

james-bebbington

Would recommend moving the config package into the subprocessmanager package as I don't think this needs to be separated

mtwo · 2020-07-29T21:07:59Z

Assigning to @keitwb for review, as per Jay's suggestion during the weekly meeting

keitwb · 2020-07-30T13:44:53Z

receiver/prometheusexecreceiver/subprocessmanager/manager.go

+// Process struct holds all the info needed to instantiate a subprocess
+type Process struct {
+	Command string
+	Port    int


Is Port part of some yet unimplemented feature? I can't see any uses of it. Is it supposed to be used to construct an arg to the subprocess telling it what port to run on?

Yeah! This PR is one part of the bigger picture receiver prometheus_exec and port is important! thanks! It's used to keep track of the original port passed into the config for this process/receiver combo.

EDIT: removing since this package is being moved to the Core repo, and I want to make it usable by all

receiver/prometheusexecreceiver/subprocessmanager/manager.go

Nicolas-MacBeth · 2020-08-03T22:08:11Z

This package is moving to the core repo. This decision was following discussions with @bogdandrutu and @james-bebbington to make this usable by anyone who needs to manage subprocesses in their code.

bogdandrutu · 2020-08-05T02:12:12Z

@Nicolas-MacBeth to unblock you, we thing it is better if you work in parallel:

Move to the next PR after this - I will merge the this PR
In parallel discuss the core PR and then later comeback and update this to use that.

Nicolas-MacBeth · 2020-08-05T17:08:35Z

@Nicolas-MacBeth to unblock you, we thing it is better if you work in parallel:

Move to the next PR after this - I will merge the this PR

In parallel discuss the core PR and then later comeback and update this to use that.

Sounds good! Thanks for the swift responses.

… done during a request (#499) Co-authored-by: alrex <aboten@lightstep.com>

[![Mend Renovate logo banner](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/pkg/sftp](https://togithub.com/pkg/sftp) | require | patch | `v1.13.5` -> `v1.13.6` | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>pkg/sftp (github.com/pkg/sftp)</summary> ### [`v1.13.6`](https://togithub.com/pkg/sftp/releases/tag/v1.13.6) [Compare Source](https://togithub.com/pkg/sftp/compare/v1.13.5...v1.13.6) \[[GH-499](https://togithub.com/pkg/sftp/issues/499)] writeToSequential: improve tests for write errors \[[GH-513](https://togithub.com/pkg/sftp/issues/513)] More context for EOF during client setup \[[GH-516](https://togithub.com/pkg/sftp/issues/516)] RealPathFileLister: allow to return an error \[[GH-525](https://togithub.com/pkg/sftp/issues/525)] Document the weirdness of the reversal of arguments to SSH_FXP_SYMLINK \[[GH-526](https://togithub.com/pkg/sftp/issues/526)] request server: handle relative symlinks \[[GH-528](https://togithub.com/pkg/sftp/issues/528)] Add support for working directory in Server \[[GH-533](https://togithub.com/pkg/sftp/issues/533)] CI: add CIFuzz integration \[[GH-537](https://togithub.com/pkg/sftp/issues/537)] Stop ReadFromWithConcurrency sending more data than it needs to \[[GH-545](https://togithub.com/pkg/sftp/issues/545)] refactor sshfx encoding, fix link rot, go fmt \[[GH-553](https://togithub.com/pkg/sftp/issues/553)] Marshal extended attribute data if FileInfo supports it \[[GH-554](https://togithub.com/pkg/sftp/issues/554)] Properly handle io.EOF error conditions when reading </details> --- ### Configuration 📅 **Schedule**: Branch creation - "on tuesday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/open-telemetry/opentelemetry-collector-contrib).  --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: opentelemetrybot <107717825+opentelemetrybot@users.noreply.github.com>

…ry#29477) [![Mend Renovate logo banner](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/pkg/sftp](https://togithub.com/pkg/sftp) | require | patch | `v1.13.5` -> `v1.13.6` | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>pkg/sftp (github.com/pkg/sftp)</summary> ### [`v1.13.6`](https://togithub.com/pkg/sftp/releases/tag/v1.13.6) [Compare Source](https://togithub.com/pkg/sftp/compare/v1.13.5...v1.13.6) \[[open-telemetryGH-499](https://togithub.com/pkg/sftp/issues/499)] writeToSequential: improve tests for write errors \[[open-telemetryGH-513](https://togithub.com/pkg/sftp/issues/513)] More context for EOF during client setup \[[open-telemetryGH-516](https://togithub.com/pkg/sftp/issues/516)] RealPathFileLister: allow to return an error \[[open-telemetryGH-525](https://togithub.com/pkg/sftp/issues/525)] Document the weirdness of the reversal of arguments to SSH_FXP_SYMLINK \[[open-telemetryGH-526](https://togithub.com/pkg/sftp/issues/526)] request server: handle relative symlinks \[[open-telemetryGH-528](https://togithub.com/pkg/sftp/issues/528)] Add support for working directory in Server \[[open-telemetryGH-533](https://togithub.com/pkg/sftp/issues/533)] CI: add CIFuzz integration \[[open-telemetryGH-537](https://togithub.com/pkg/sftp/issues/537)] Stop ReadFromWithConcurrency sending more data than it needs to \[[open-telemetryGH-545](https://togithub.com/pkg/sftp/issues/545)] refactor sshfx encoding, fix link rot, go fmt \[[open-telemetryGH-553](https://togithub.com/pkg/sftp/issues/553)] Marshal extended attribute data if FileInfo supports it \[[open-telemetryGH-554](https://togithub.com/pkg/sftp/issues/554)] Properly handle io.EOF error conditions when reading </details> --- ### Configuration 📅 **Schedule**: Branch creation - "on tuesday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/open-telemetry/opentelemetry-collector-contrib).  --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: opentelemetrybot <107717825+opentelemetrybot@users.noreply.github.com>

config, subprocess manager and tests

e8068e8

Nicolas-MacBeth mentioned this pull request Jul 24, 2020

OpenTelemetry Collector Ecosystem Integration #370

Closed

james-bebbington reviewed Jul 24, 2020

View reviewed changes

Nicolas-MacBeth added 2 commits July 26, 2020 19:05

Changed the folder name, and made small changes according to James' c…

d4be2f9

…omments

merge conflict resolved in go.sum

5050290

james-bebbington reviewed Jul 26, 2020

View reviewed changes

go.sum Outdated Show resolved Hide resolved

Removed changes in global go.sum

05686b5

Nicolas-MacBeth marked this pull request as ready for review July 27, 2020 15:57

Nicolas-MacBeth requested a review from a team July 27, 2020 15:57

james-bebbington approved these changes Jul 27, 2020

View reviewed changes

took out config from its own package

d83f304

mtwo assigned keitwb Jul 29, 2020

keitwb reviewed Jul 30, 2020

View reviewed changes

receiver/prometheusexecreceiver/subprocessmanager/manager.go Show resolved Hide resolved

Nicolas-MacBeth closed this Aug 3, 2020

bogdandrutu reopened this Aug 5, 2020

bogdandrutu merged commit ff7ab28 into open-telemetry:master Aug 5, 2020

james-bebbington mentioned this pull request Aug 5, 2020

subprocess manager for prometheus_exec receiver open-telemetry/opentelemetry-collector#1483

Closed

codeboten pushed a commit that referenced this pull request Nov 23, 2022

Reorder on_finish call order to correctly instrument all tornado work…

fe4e2d4

… done during a request (#499) Co-authored-by: alrex <aboten@lightstep.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subprocess manager and its tests for the prometheus_exec Receiver #499

Subprocess manager and its tests for the prometheus_exec Receiver #499

Nicolas-MacBeth commented Jul 23, 2020 •

edited

Loading

codecov bot commented Jul 23, 2020 •

edited

Loading

james-bebbington left a comment

james-bebbington Jul 24, 2020

Nicolas-MacBeth Jul 26, 2020

james-bebbington Jul 24, 2020 •

edited

Loading

Nicolas-MacBeth Jul 26, 2020

Nicolas-MacBeth commented Jul 27, 2020

james-bebbington left a comment

mtwo commented Jul 29, 2020 •

edited

Loading

keitwb Jul 30, 2020

Nicolas-MacBeth Jul 30, 2020 •

edited

Loading

Nicolas-MacBeth Aug 3, 2020

Nicolas-MacBeth commented Aug 3, 2020

bogdandrutu commented Aug 5, 2020

Nicolas-MacBeth commented Aug 5, 2020

	// GetDelay will compute the exponential backoff for a given process according to its crash count and time alive
	// GetDelay will compute the delay for a given process according to its crash count and time alive using an exponential backoff algorithm

Subprocess manager and its tests for the prometheus_exec Receiver #499

Subprocess manager and its tests for the prometheus_exec Receiver #499

Conversation

Nicolas-MacBeth commented Jul 23, 2020 • edited Loading

codecov bot commented Jul 23, 2020 • edited Loading

Codecov Report

james-bebbington left a comment

Choose a reason for hiding this comment

james-bebbington Jul 24, 2020

Choose a reason for hiding this comment

Nicolas-MacBeth Jul 26, 2020

Choose a reason for hiding this comment

james-bebbington Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

Nicolas-MacBeth Jul 26, 2020

Choose a reason for hiding this comment

Nicolas-MacBeth commented Jul 27, 2020

james-bebbington left a comment

Choose a reason for hiding this comment

mtwo commented Jul 29, 2020 • edited Loading

keitwb Jul 30, 2020

Choose a reason for hiding this comment

Nicolas-MacBeth Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

Nicolas-MacBeth Aug 3, 2020

Choose a reason for hiding this comment

Nicolas-MacBeth commented Aug 3, 2020

bogdandrutu commented Aug 5, 2020

Nicolas-MacBeth commented Aug 5, 2020

Nicolas-MacBeth commented Jul 23, 2020 •

edited

Loading

codecov bot commented Jul 23, 2020 •

edited

Loading

james-bebbington Jul 24, 2020 •

edited

Loading

mtwo commented Jul 29, 2020 •

edited

Loading

Nicolas-MacBeth Jul 30, 2020 •

edited

Loading