Follow logs using overlap #89

nocturnalastro · 2023-09-14T13:41:03Z

No description provided.

natifridman · 2023-09-20T12:03:12Z

pkg/cmd/collect.go

+			usr, err := user.Current()
+			if err != nil {
+				log.Fatal("Failed to result homedir for current user in --tempdir")


Since the error is for user.Current() maybe we should rephrase the error message? something like

Suggested change

usr, err := user.Current()

if err != nil {

log.Fatal("Failed to result homedir for current user in --tempdir")

usr, err := user.Current()

if err != nil {

log.Fatal("Failed to get current user")

Changed to "Failed to fetch current user so could not resolve tempdir"

natifridman · 2023-09-20T12:08:59Z

pkg/runner/collector_selector.go

@@ -13,13 +13,16 @@ import (
 var (
 	OptionalCollectorNames []string
 	RequiredCollectorNames []string
+	OptInCollectorNames    []string


Not sure I understand from the name what it is. Sounds very similar to OptionalCollectorNames

OptIn is off by default, Optional is on by default but can be turned off.

I can probably take this out as we moved the log collector to on by default

crwr45

needs a pass to rewrite the test descriptions and some comments for clarity, but looks promising.

crwr45 · 2023-10-05T11:55:27Z

pkg/cmd/collect.go

+	collectCmd.Flags().StringVarP(
+		&logsOutputFile,
+		"logs-output", "l", "",
+		"Path to the logs output file. This is required when using the opt in logs collector",


Suggested change

"Path to the logs output file. This is required when using the opt in logs collector",

"Path to the logs output file. This is required when using the logs collector",

crwr45 · 2023-10-05T14:33:07Z

pkg/collectors/log_follower.go

+)
+
+// LogsCollector collects logs from repeated calls to the kubeapi with overlapping query times,
+// the lines are then fed into a channel, in another gorotine they are de-duplicated and written to an output file.


Suggested change

// the lines are then fed into a channel, in another gorotine they are de-duplicated and written to an output file.

// the lines are then fed into a channel, in another goroutine they are de-duplicated and written to an output file.

crwr45 · 2023-10-05T15:20:00Z

pkg/loglines/dedup.go

+}
+
+//nolint:varnamelen // x and y are just two sets of lines
+func findIncompatableLines(x, y []*ProcessedLine) int {


Fix typo in name and add a definition of what an incompatible line is as it's extensively used:

Suggested change

func findIncompatableLines(x, y []*ProcessedLine) int {

// an incompatible line is a line that exists in x without being present at the same index in y.

func findIncompatibleLines(x, y []*ProcessedLine) int {

also consider renaming to something like findFirstIssueIndex

crwr45 · 2023-10-05T15:22:08Z

pkg/loglines/dedup.go

+}
+
+//nolint:gocritic,varnamelen // don't want to name the return values as they should be built later, I think x, y are expressive enough names
+func fixLines(x, y []*ProcessedLine, issueIndex int) ([]*ProcessedLine, []*ProcessedLine, bool) {


fixLines needs a sentence or two docstring explaining what the two output line slices are and what guarantees are made about them

crwr45 · 2023-10-05T15:24:12Z

pkg/loglines/dedup_test.go

+	return loglines.MakeSliceFromLines(lines, generation), nil
+}
+
+var _ = Describe("Dedup AB tests", func() {


the text in each of the When and It calls needs to be rewritten for clarity

I've gone through them if you think there needs extra let me know.

collectorInclusionType has 3 values require, includeByDefault and optIn

…ualt names - Change all to defaults and include opt in collectors for All

crwr45

Looks good, happy to merge once we have a long run that matches the more crude method.

nocturnalastro · 2023-10-17T08:22:04Z

Did 3000m and completely matched the crude method :)

nocturnalastro force-pushed the followLogsOverlap branch 2 times, most recently from 54e4b29 to 47b5813 Compare September 20, 2023 10:23

nocturnalastro marked this pull request as ready for review September 20, 2023 11:35

natifridman reviewed Sep 20, 2023

View reviewed changes

nocturnalastro force-pushed the followLogsOverlap branch from 57b5836 to 18c94b6 Compare October 5, 2023 12:41

crwr45 suggested changes Oct 5, 2023

View reviewed changes

nocturnalastro added 6 commits October 6, 2023 17:41

Inital log follower collector

ada40fe

Add opt in collectors

5e9b8c2

Add collectorInclusionType for collector registry

c71eb74

collectorInclusionType has 3 values require, includeByDefault and optIn

Add log timestamp flag for collectors

b8c0c9f

Don't short circuit collector names and change collector selector def…

a5aadfe

…ualt names - Change all to defaults and include opt in collectors for All

Remove the log subcommand in favour of the logs collector

102f134

nocturnalastro force-pushed the followLogsOverlap branch from 18c94b6 to 2668088 Compare October 6, 2023 16:41

nocturnalastro added 3 commits October 9, 2023 14:20

Dedup using overlaping generations as it provides better consitancey

44ced6c

Improve error message when resolving user's homedir

4963c60

Remove unused OptIn collector catagory

128781e

nocturnalastro force-pushed the followLogsOverlap branch from cff147b to 128781e Compare October 9, 2023 13:20

nocturnalastro requested a review from crwr45 October 9, 2023 14:33

crwr45 approved these changes Oct 10, 2023

View reviewed changes

nocturnalastro merged commit d7ab104 into redhat-partner-solutions:main Oct 17, 2023
5 checks passed

nocturnalastro deleted the followLogsOverlap branch October 17, 2023 08:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Follow logs using overlap #89

Follow logs using overlap #89

nocturnalastro commented Sep 14, 2023

natifridman Sep 20, 2023

nocturnalastro Oct 3, 2023

natifridman Sep 20, 2023

nocturnalastro Sep 20, 2023

nocturnalastro Sep 20, 2023

nocturnalastro Oct 3, 2023

crwr45 left a comment

crwr45 Oct 5, 2023

crwr45 Oct 5, 2023

crwr45 Oct 5, 2023

crwr45 Oct 5, 2023

crwr45 Oct 5, 2023

crwr45 Oct 5, 2023

nocturnalastro Oct 9, 2023

crwr45 left a comment

nocturnalastro commented Oct 17, 2023

	"Path to the logs output file. This is required when using the opt in logs collector",
	"Path to the logs output file. This is required when using the logs collector",

	// the lines are then fed into a channel, in another gorotine they are de-duplicated and written to an output file.
	// the lines are then fed into a channel, in another goroutine they are de-duplicated and written to an output file.

	func findIncompatableLines(x, y []*ProcessedLine) int {
	// an incompatible line is a line that exists in x without being present at the same index in y.
	func findIncompatibleLines(x, y []*ProcessedLine) int {

Follow logs using overlap #89

Follow logs using overlap #89

Conversation

nocturnalastro commented Sep 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crwr45 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crwr45 left a comment

Choose a reason for hiding this comment

nocturnalastro commented Oct 17, 2023