add the NewPidFileFn to helper #804

sbookworm · 2020-10-10T08:45:32Z

for the enhancement: #760

beorn7

Thank you for your contribution. It looks good in general. Just minor comments below.

Another thing is the file naming. helper.go suggest we will put more helper function in there later. I would either call the file pid_file_helper.go (and the test file accordingly), or perhaps even better, just add it to process_collector.go, because it's not that much new code, and it is closely tied to the code in process_collector.go.

prometheus/helper.go

beorn7 · 2020-10-12T12:51:50Z

prometheus/helper.go

+		}
+		value, err := strconv.Atoi(strings.TrimSpace(string(content)))
+		if err != nil {
+			return 0, fmt.Errorf("can't parse pid file %s with error: %+v", pidFilePath, err)


Same comment as above.

prometheus/helper.go

beorn7 · 2020-10-12T12:57:33Z

prometheus/helper_test.go

+		t.Error("failed to get current path")
+	}
+	mockPidFilePath := filepath.Join(folderPath, "mockPidFile")
+	defer cleanMockPidFile(mockPidFilePath)


Is there any harm in calling os.Remove on a non-existing file? It should just be a no-op. I'd replace cleanMockPidFile(mockPidFilePath) by os.Remove(mockPidFilePath) everywhere and get rid of the helper functions fileExist and cleanMockPidFile.

good idea 👍

sbookworm · 2020-10-20T12:18:16Z

@beorn7 sorry for using the force option when submitting the changes.
There is only one latest commit now :( please help to review, thanks

beorn7

Code looks good now. But you still have it in files called helper.go and helper_test.go, which suggests those are files for helper code. But there are no plans right now to add more "helper" code to them. My suggestion is to move the code from helper.go to process_collector.go and the code from helper_test.go to process_collector_test.go.

beorn7 · 2020-10-20T16:13:55Z

sorry for using the force option when submitting the changes

That's fine for your own branch if it is unlikely that those changes are used somewhere else already.

beorn7

Thank you. Just one final thing to do.

prometheus/process_collector_test.go

Signed-off-by: sbookworm <lisong.cruise@gmail.com>

beorn7

Thank you.

sbookworm force-pushed the get_pid_from_file branch from 266d6df to cf71794 Compare October 10, 2020 08:50

sbookworm mentioned this pull request Oct 10, 2020

Add PidFile option #760

Closed

SuperQ requested review from SuperQ and beorn7 October 10, 2020 09:18

beorn7 requested changes Oct 12, 2020

View reviewed changes

sbookworm force-pushed the get_pid_from_file branch 2 times, most recently from bba4f5d to 5bfc9f3 Compare October 20, 2020 12:16

sbookworm requested a review from beorn7 October 20, 2020 12:27

beorn7 requested changes Oct 20, 2020

View reviewed changes

sbookworm force-pushed the get_pid_from_file branch from 5bfc9f3 to 317d499 Compare October 22, 2020 13:26

sbookworm requested a review from beorn7 October 22, 2020 15:32

beorn7 requested changes Oct 22, 2020

View reviewed changes

prometheus/process_collector_test.go Outdated Show resolved Hide resolved

add the NewPidFileFn to helper

469ec27

Signed-off-by: sbookworm <lisong.cruise@gmail.com>

sbookworm force-pushed the get_pid_from_file branch from 317d499 to 469ec27 Compare October 23, 2020 01:24

sbookworm requested a review from beorn7 October 23, 2020 01:46

beorn7 approved these changes Oct 23, 2020

View reviewed changes

beorn7 merged commit f97b033 into prometheus:master Oct 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add the NewPidFileFn to helper #804

add the NewPidFileFn to helper #804

sbookworm commented Oct 10, 2020

beorn7 left a comment

beorn7 Oct 12, 2020

beorn7 Oct 12, 2020

sbookworm Oct 20, 2020

sbookworm commented Oct 20, 2020

beorn7 left a comment

beorn7 commented Oct 20, 2020

beorn7 left a comment

beorn7 left a comment

add the NewPidFileFn to helper #804

add the NewPidFileFn to helper #804

Conversation

sbookworm commented Oct 10, 2020

beorn7 left a comment

Choose a reason for hiding this comment

beorn7 Oct 12, 2020

Choose a reason for hiding this comment

beorn7 Oct 12, 2020

Choose a reason for hiding this comment

sbookworm Oct 20, 2020

Choose a reason for hiding this comment

sbookworm commented Oct 20, 2020

beorn7 left a comment

Choose a reason for hiding this comment

beorn7 commented Oct 20, 2020

beorn7 left a comment

Choose a reason for hiding this comment

beorn7 left a comment

Choose a reason for hiding this comment