Make k6 wait for loki to finish pushing before stopping #1694

mstoykov · 2020-10-29T10:00:51Z

No description provided.

mstoykov · 2020-10-30T11:20:55Z

This is dependant on #1691

This is especially useful if there was an error that stopped the execution really fast but loki wouldn't had time to log it before it was stopped

codecov-io · 2020-11-02T17:45:21Z

Codecov Report

Merging #1694 (076a7dd) into master (c51095a) will decrease coverage by 0.03%.
The diff coverage is 10.38%.

@@            Coverage Diff             @@
##           master    #1694      +/-   ##
==========================================
- Coverage   71.42%   71.39%   -0.04%     
==========================================
  Files         176      178       +2     
  Lines       13681    13751      +70     
==========================================
+ Hits         9772     9817      +45     
- Misses       3298     3322      +24     
- Partials      611      612       +1

Flag	Coverage Δ
ubuntu	`71.35% <10.38%> (-0.03%)`	⬇️
windows	`69.92% <9.09%> (-0.05%)`	⬇️

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

Impacted Files	Coverage Δ
cmd/root.go	`8.75% <4.22%> (-1.59%)`	⬇️
log/loki.go	`38.86% <75.00%> (+0.69%)`	⬆️
js/modules/k6/http/request.go	`80.64% <100.00%> (ø)`
core/engine.go	`90.95% <0.00%> (-2.02%)`	⬇️
stats/cloud/collector.go	`79.81% <0.00%> (-0.64%)`	⬇️
js/initcontext.go	`92.13% <0.00%> (ø)`
js/modules/k6/k6.go	`100.00% <0.00%> (ø)`
js/common/context.go	`100.00% <0.00%> (ø)`
js/modules/k6/grpc/grpc.go	`100.00% <0.00%> (ø)`
js/modules/k6/http/http.go	`100.00% <0.00%> (ø)`
... and 12 more

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 c51095a...076a7dd. Read the comment docs.

imiric

LGTM, just some questions and nitpicks.

imiric · 2020-11-02T11:22:48Z

cmd/root.go

-		if err != nil {
-			return err
-		}
+// This is get all for the main/root k6 command struct


I don't understand this comment. Typo?

I think I meant to write

This is to get all fields for the main/root k6 command struct

but I eat half the words ? I remember that this strut was 4 things in the begining so 🤷

imiric · 2020-11-04T10:42:25Z

cmd/root.go

+		Long:              BannerColor.Sprintf("\n%s", consts.Banner()),
+		SilenceUsage:      true,
+		SilenceErrors:     true,
+		PersistentPreRunE: c.persistentPerRunE,


Suggested change

PersistentPreRunE: c.persistentPerRunE,

PersistentPreRunE: c.persistentPreRunE,

imiric · 2020-11-04T10:48:45Z

cmd/root.go

+		// TODO: figure out something else... currently, with the wrappers
+		// below, we're stripping any colors from the output after we've
+		// added them. The problem is that, besides being very inefficient,
+		// this actually also strips other special characters from the
+		// intended output, like the progressbar formatting ones, which
+		// would otherwise be fine (in a TTY).
+		//
+		// It would be much better if we avoid messing with the output and
+		// instead have a parametrized instance of the color library. It
+		// will return colored output if colors are enabled and simply
+		// return the passed input as-is (i.e. be a noop) if colors are
+		// disabled...


Strong 👍 for fixing this. I didn't find an existing GH issue for it, so can you create one and link it here instead of the explanation?

I just moved this around ... this was written by @na-- a while ago a9cc130 😉 I don't think having this in an issue will help anyone, if you start working on it, maybe make one and we can discuss it but in general I consider this a refactoring and having refactoring issues given the amount of issues we have that IMO we will never actually work on .. so they are basically "ideas" makes the issues hard to use so ... yeah, I don't think this needs to be added to an issue.

I myself have added issues (mostly refactoring ones) that again are hardly unlikely for us to ever work on ... and even if we did the fact that there is an issue would've not helped in any way.

On related note, I have a branch with 1 more commit where I get EVEN more of the global variables like noColor in a struct and get it passed around, but that will have to wait for v0.30.0 maybe I can try this then

imiric · 2020-11-04T10:54:03Z

cmd/root.go

+	// RootCmd represents the base command when called without any subcommands.
+	c.cmd = &cobra.Command{


There's no RootCmd anymore, and I find the generic newCommand/command names slightly misleading since it's only for the root command, but maybe just:

Suggested change

// RootCmd represents the base command when called without any subcommands.

c.cmd = &cobra.Command{

// The base command when called without any subcommands.

c.cmd = &cobra.Command{

cmd/root.go

imiric · 2020-11-04T11:31:22Z

log/loki.go

@@ -282,7 +289,7 @@ func (h *lokiHook) loop() {
 		case <-h.ctx.Done():
 			ch := make(chan int64)
 			pushCh <- ch
-			ch <- 0
+			ch <- time.Now().Add(time.Second).UnixNano()


Since this is the cutoff time, is the second to deal with time sync issues and get some last events? Should it be equal to the push period, so maybe h.pushPeriod here instead?

yeah ... the basic idea is that ... as far as I can see you can call time.Now() in two parallel goroutines ... and you won't exactly get ... sequential times even if in reality they were ... so I added some time to get "hopefully" everything.

I don't think that h.pushPeriod is better ... if anything I am more for time.Hour and a comment explaining why it is done at all (this took me 1 hour to figure out why I wasn't getting the last messages even though I was waiting for them and around 10 fmt.Printlns)

na--

LGTM, though I'd prefer if we have the global timeout as well... And I'm not sure I like the rootCommand struct, but I guess it's better than globals and we're restricted by the structure imposed from cobra, so it's fine for now...

na-- · 2020-11-24T11:39:02Z

cmd/root.go

+		Long:              BannerColor.Sprintf("\n%s", consts.Banner()),
+		SilenceUsage:      true,
+		SilenceErrors:     true,
+		PersistentPreRunE: c.persistentPreRunE,
 	}

 	confDir, err := os.UserConfigDir()


At some later point in this refactoring series, we should probably have an interface with some of the same methods as the os package (Environ(), Getwd() and the other FS ones, etc.) and pass that in as a parameter to these constructors...

Hm ... that is a good idea

cmd/root.go

imiric

The 1s delay to get the last messages seems a bit arbitrary, and maybe we should increase it or consider more robust solutions, but we can come back to it if needed, so LGTM.

mstoykov requested review from imiric and na-- and removed request for imiric October 29, 2020 15:49

mstoykov added this to the v0.29.0 milestone Oct 30, 2020

mstoykov force-pushed the cmdRefactoring branch from 79ffeda to 6475b7a Compare October 30, 2020 13:33

mstoykov mentioned this pull request Oct 30, 2020

Print errors stopping the execution to the stderr #1690

Closed

mstoykov force-pushed the cmdRefactoring branch from ec3d598 to d9381c0 Compare November 2, 2020 11:22

Base automatically changed from cmdRefactoring to master November 2, 2020 11:40

Make k6 wait for loki to finish pushing before stopping

44627c9

This is especially useful if there was an error that stopped the execution really fast but loki wouldn't had time to log it before it was stopped

mstoykov force-pushed the waitLokiBeforeExiting branch from 96f24bf to 44627c9 Compare November 2, 2020 17:45

imiric reviewed Nov 4, 2020

View reviewed changes

Some renames and comment fixes

170eafb

mstoykov modified the milestones: v0.29.0, v0.30.0 Nov 4, 2020

na-- previously approved these changes Nov 24, 2020

View reviewed changes

Add remote logger wait timeout

076a7dd

mstoykov dismissed na--’s stale review via 076a7dd November 25, 2020 12:39

mstoykov requested review from imiric and na-- November 25, 2020 12:39

na-- approved these changes Nov 25, 2020

View reviewed changes

imiric approved these changes Nov 25, 2020

View reviewed changes

mstoykov merged commit 62665cf into master Nov 25, 2020

mstoykov deleted the waitLokiBeforeExiting branch November 25, 2020 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make k6 wait for loki to finish pushing before stopping #1694

Make k6 wait for loki to finish pushing before stopping #1694

mstoykov commented Oct 29, 2020

mstoykov commented Oct 30, 2020

codecov-io commented Nov 2, 2020 •

edited

imiric left a comment

imiric Nov 2, 2020

mstoykov Nov 4, 2020

imiric Nov 4, 2020

imiric Nov 4, 2020

mstoykov Nov 4, 2020

imiric Nov 4, 2020

imiric Nov 4, 2020

mstoykov Nov 4, 2020

na-- left a comment

na-- Nov 24, 2020

mstoykov Nov 24, 2020

imiric left a comment

	PersistentPreRunE: c.persistentPerRunE,
	PersistentPreRunE: c.persistentPreRunE,

		// RootCmd represents the base command when called without any subcommands.
		c.cmd = &cobra.Command{

Make k6 wait for loki to finish pushing before stopping #1694

Make k6 wait for loki to finish pushing before stopping #1694

Conversation

mstoykov commented Oct 29, 2020

mstoykov commented Oct 30, 2020

codecov-io commented Nov 2, 2020 • edited

Codecov Report

imiric 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

Choose a reason for hiding this comment

na-- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imiric left a comment

Choose a reason for hiding this comment

codecov-io commented Nov 2, 2020 •

edited