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

build: attach name of pipeline step to summary #4396

Merged
merged 5 commits into from
Apr 6, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 29 additions & 10 deletions internal/build/pipeline_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,26 @@ import (
)

type PipelineState struct {
curPipelineStep int
curPipelineStep PipelineStep
curBuildStep int
totalPipelineStepCount int
pipelineStepDurations []time.Duration
pipelineSteps []PipelineStep
curPipelineStart time.Time
curPipelineStepStart time.Time
c Clock
}

type PipelineStep struct {
Name string // for logging
Index int // human-readable, 1-indexed
Duration time.Duration // not populated until end of the step
}

func (pStep PipelineStep) withDuration(d time.Duration) PipelineStep {
pStep.Duration = d
return pStep
}

type Clock interface {
Now() time.Time
}
Expand All @@ -35,7 +46,7 @@ const buildStepOutputPrefix = " "

func NewPipelineState(ctx context.Context, totalStepCount int, c Clock) *PipelineState {
return &PipelineState{
curPipelineStep: 1,
curPipelineStep: PipelineStep{Index: 0},
Copy link
Member

Choose a reason for hiding this comment

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

IIUC it's idiomatic go to omit initializations to 0-values like this (both the Index and the PipelineStep here are such initializations)

(this is irrelevant if you agree with my below comment to delete curPipelineStep)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ha whoops yes. it used to be initialized to 1 and then i changed it but heh.

totalPipelineStepCount: totalStepCount,
curPipelineStart: c.Now(),
c: c,
Expand All @@ -48,7 +59,7 @@ func NewPipelineState(ctx context.Context, totalStepCount int, c Clock) *Pipelin
// and NOT:
// defer ps.End(ctx, err)
func (ps *PipelineState) End(ctx context.Context, err error) {
ps.curPipelineStep = 0
ps.curPipelineStep = PipelineStep{Index: 0}
ps.curBuildStep = 0

if err != nil {
Expand All @@ -59,27 +70,35 @@ func (ps *PipelineState) End(ctx context.Context, err error) {

elapsed := ps.c.Now().Sub(ps.curPipelineStart)

for i, duration := range ps.pipelineStepDurations {
l.Infof("%sStep %d - %.2fs", buildStepOutputPrefix, i+1, duration.Seconds())
for i, step := range ps.pipelineSteps {
l.Infof("%sStep %d - %.2fs (%s)", buildStepOutputPrefix, i+1, step.Duration.Seconds(), step.Name)
}

t := logger.Blue(l).Sprintf("%.2fs", elapsed.Seconds())
l.Infof("%sDONE IN: %s \n", buildStepOutputPrefix, t)
}

func (ps *PipelineState) curPipelineIndex() int {
return ps.curPipelineStep.Index
}

func (ps *PipelineState) StartPipelineStep(ctx context.Context, format string, a ...interface{}) {
l := logger.Get(ctx)
line := logger.Blue(l).Sprintf("STEP %d/%d", ps.curPipelineStep, ps.totalPipelineStepCount)
l.Infof("%s — %s", line, fmt.Sprintf(format, a...))
ps.curPipelineStep++
stepName := fmt.Sprintf(format, a...)
ps.curPipelineStep = PipelineStep{
Name: stepName,
Index: ps.curPipelineIndex() + 1,
}
line := logger.Blue(l).Sprintf("STEP %d/%d", ps.curPipelineIndex(), ps.totalPipelineStepCount)
Copy link
Member

Choose a reason for hiding this comment

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

Would this be simpler?

  1. remove the PipelineStep.Index field
  2. remove curPiplineIndex and replacing with len(ps.pipelineSteps) (or with + 1 or whatever)
  3. remove curPiplineStep, and the new step immediately, and use pipelineSteps[len(pipelineSteps)-1] in EndPipelineStep (which I think is the only place this gets used?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yyyup good call

l.Infof("%s — %s", line, stepName)
ps.curBuildStep = 1
ps.curPipelineStepStart = ps.c.Now()
Copy link
Member

Choose a reason for hiding this comment

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

wanna pull this field into the PipelineStep struct?

}

func (ps *PipelineState) EndPipelineStep(ctx context.Context) {
elapsed := ps.c.Now().Sub(ps.curPipelineStepStart)
logger.Get(ctx).Infof("")
ps.pipelineStepDurations = append(ps.pipelineStepDurations, elapsed)
ps.pipelineSteps = append(ps.pipelineSteps, ps.curPipelineStep.withDuration(elapsed))
}

func (ps *PipelineState) StartBuildStep(ctx context.Context, format string, a ...interface{}) {
Expand Down
20 changes: 20 additions & 0 deletions internal/build/pipeline_state_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,26 @@ func TestPipelineMultilinePrint(t *testing.T) {
assertSnapshot(t, out.String())
}

func TestPipelineMultistep(t *testing.T) {
var err error
out := &bytes.Buffer{}
ctx := logger.WithLogger(context.Background(), logger.NewLogger(logger.InfoLvl, out))
ps := NewPipelineState(ctx, 3, fakeClock{})
ps.StartPipelineStep(ctx, "%s %s", "hello", "world")
ps.Printf(ctx, "in ur step")
ps.EndPipelineStep(ctx)
ps.StartPipelineStep(ctx, "doing stuff")
ps.Printf(ctx, "here is some stuff!")
ps.EndPipelineStep(ctx)
ps.StartPipelineStep(ctx, "different stuff")
ps.Printf(ctx, "even more stuff")
ps.Printf(ctx, "i can't believe it's not stuff")
ps.EndPipelineStep(ctx)
ps.End(ctx, err)

assertSnapshot(t, out.String())
}

func assertSnapshot(t *testing.T, output string) {
d1 := []byte(output)
gmPath := fmt.Sprintf("testdata/%s_master", t.Name())
Expand Down
2 changes: 1 addition & 1 deletion internal/build/testdata/TestPipelineMultilinePrint_master
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@ STEP 1/1 — hello world
line 2


Step 1 - 0.00s
Step 1 - 0.00s (hello world)
DONE IN: 0.00s

15 changes: 15 additions & 0 deletions internal/build/testdata/TestPipelineMultistep_master
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
STEP 1/3 — hello world
in ur step

STEP 2/3 — doing stuff
here is some stuff!

STEP 3/3 — different stuff
even more stuff
i can't believe it's not stuff

Step 1 - 0.00s (hello world)
Step 2 - 0.00s (doing stuff)
Step 3 - 0.00s (different stuff)
DONE IN: 0.00s

2 changes: 1 addition & 1 deletion internal/build/testdata/TestPipeline_master
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
STEP 1/1 — hello world
in ur step

Step 1 - 0.00s
Step 1 - 0.00s (hello world)
DONE IN: 0.00s

3 changes: 3 additions & 0 deletions web/src/subdir/index.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
test("everything is okay", () => {
Copy link
Member

Choose a reason for hiding this comment

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

👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is fine

expect(1).toEqual(1)
});