Fix Annotee stream name generation.

logdog/client/annotee/annotation/annotation.go

 // Copyright 2015 The LUCI Authors. All rights reserved.
 // Use of this source code is governed under the Apache License, Version 2.0
 // that can be found in the LICENSE file.
 
 package annotation
 
 import (
         "fmt"
-        "path"
         "strconv"
         "strings"
         "time"
 
         "github.com/luci/luci-go/common/clock"
         "github.com/luci/luci-go/common/proto/google"
         "github.com/luci/luci-go/common/proto/milo"
         "github.com/luci/luci-go/logdog/common/types"
 )
 
@@ skipping 73 matching lines @@
                 return
         }
 
         s.stepMap = map[string]*Step{}
         s.stepLookup = map[*milo.Step]*Step{}
 
         name := "steps"
         if s.Execution != nil {
                 name = s.Execution.Name
         }
-        s.rootStep.initializeStep(s, nil, name, 0, s.LogNameBase)
+        s.rootStep.initializeStep(s, nil, name)
+        s.rootStep.LogNameBase = s.LogNameBase
         s.SetCurrentStep(nil)
 
         // Add our Command parameters, if applicable.
         if exec := s.Execution; exec != nil {
                 s.rootStep.Command = &milo.Step_Command{
                         CommandLine: exec.Command,
                         Cwd:         exec.Dir,
                         Environ:     exec.Env,
                 }
         }
@@ skipping 415 matching lines @@
         // Both prevStep and nextStep are creation-ordered, and don't change even if
         // a Step is reparented.
         prevStep *Step
         // nextStep is the step that was created immediately after this step. It is
         // nil if this is the latest step.
         //
         // Both prevStep and nextStep are creation-ordered, and don't change even if
         // a Step is reparented.
         nextStep *Step
 
-        index int
         level int
 
-        stepIndex map[string]int
+        // logPathIndex is a map of the number of log paths with the given base name.
+        // Each time a log path is generated, it will register with this map and
+        // increase the count.
+        logPathIndex map[types.StreamName]int
 
         // logLines is a map of log line labels to full log stream names.
         logLines map[string]types.StreamName
         // logLineCount is a map of log line label to the number of times that log
         // line has appeared. This is to prevent the case where multiple log lines
         // with the same label may be emitted, which would cause duplicate log stream
         // names.
         logLineCount map[string]int
 
-        // LogNameBase is the LogDog stream name root for this step.
-        logNameBase types.StreamName
+        // logNameBase is the LogDog stream name root for this step.
+        LogNameBase types.StreamName
         // hasSummary, if true, means that this Step has summary text. The summary
         // text is stored as the first line in its Step.Text slice.
         hasSummary bool
         // closed is true if the element is closed.
         closed bool
 }
 
-func (as *Step) initializeStep(s *State, parent *Step, name string, index int, logNameBase types.StreamName) *Step {
+func (as *Step) String() string { return string(as.LogNameBase) }
+
+func (as *Step) initializeStep(s *State, parent *Step, name string) *Step {
         t := milo.Status_RUNNING
         as.Step = milo.Step{
                 Name:   name,
                 Status: t,
         }
 
         as.s = s
-        as.index = index
-        as.logNameBase = logNameBase
-        as.stepIndex = map[string]int{}
         as.logLines = map[string]types.StreamName{}
         as.logLineCount = map[string]int{}
+        as.logPathIndex = map[types.StreamName]int{}
 
         // Add this Step to our parent's Substep list.
         if parent != nil {
                 parent.appendSubstep(as)
         }
         s.registerStep(as)
 
         return as
 }
 
 func (as *Step) appendSubstep(s *Step) {
         if s.parent == as {
                 // Already parented to as, so do nothing.
                 return
         }
         s.detachFromParent()
 
         s.parent = as
         as.Substep = append(as.Substep, &milo.Step_Substep{
                 Substep: &milo.Step_Substep_Step{
                         Step: &s.Step,
                 },
         })
+        s.regenerateLogPath()
 }
 
 func (as *Step) detachFromParent() {
         parent := as.parent
         if parent == nil {
                 return
         }
 
         // Remove any instances of "as" from its current parent's Substeps.
         ssPtr := 0
         for _, ss := range parent.Substep {
                 if ss.GetStep() != &as.Step {
                         parent.Substep[ssPtr] = ss
                         ssPtr++
                 }
         }
         parent.Substep = parent.Substep[:ssPtr]
         as.parent = nil
 }
 
-// CanonicalName returns the canonical name of this Step. This name is
-// guaranteed to be unique witin the State.
-func (as *Step) CanonicalName() string {
-        parts := []string(nil)
-        if as.index == 0 {
-                parts = append(parts, as.Name())
-        } else {
-                parts = append(parts, fmt.Sprintf("%s_%d", as.Name(), as.index))
-        }
-        for p := as.parent; p != nil; p = p.parent {
-                parts = append(parts, p.Name())
-        }
-        for i := len(parts)/2 - 1; i >= 0; i-- {
-                opp := len(parts) - 1 - i
-                parts[i], parts[opp] = parts[opp], parts[i]
-        }
-        return path.Join(parts...)
-}
-
 // Name returns the step's component name.
 func (as *Step) Name() string {
         return as.Step.Name
 }
 
 // Proto returns the Milo protobuf associated with this Step.
 func (as *Step) Proto() *milo.Step {
         return &as.Step
 }
 
 // BaseStream returns the supplied name prepended with this Step's base
 // log name.
 //
 // For example, if the base name is "foo/bar", BaseStream("baz") will return
 // "foo/bar/baz".
 func (as *Step) BaseStream(name types.StreamName) types.StreamName {
-        if as.logNameBase == "" {
+        if as.LogNameBase == "" {
                 return name
         }
-        return as.logNameBase.Concat(name)
+        return as.LogNameBase.Concat(name)
 }
 
 // AddStep generates a new substep.
 func (as *Step) AddStep(name string) *Step {
-        // Determine/advance step index.
-        index := as.stepIndex[name]
-        as.stepIndex[name]++
+        return (&Step{}).initializeStep(as.s, as, name)
+}
 
-        logPath, err := types.MakeStreamName("s_", "steps", name, strconv.Itoa(index))
-        if err != nil {
-                panic(fmt.Errorf("failed to generate step name for [%s]: %s", name, err))
+func (as *Step) regenerateLogPath() {
+        if as.parent == nil {
+                panic("log path regeneration cannot be called on root step")
         }
 
-        nas := (&Step{}).initializeStep(as.s, as, name, index, as.BaseStream(logPath))
-        return nas
+        // Recipe engine nests steps by prepending their parents' name, e.g.
+        // if "foo" has a nested child, it will be named "foo.bar". This is redundant
+        // for our stream names, so strip that off.
+        //
+        // We throw the length conditional in just in case the child step happens to
+        // have the exact same name as the parent. This shouldn't happen naturally,
+        // but let's be robust.
+        name := as.Name()
+        if parentPrefix := (as.parent.Name() + "."); len(parentPrefix) < len(name) {
+                name = strings.TrimPrefix(name, parentPrefix)
+        }
+
+        logPath, err := types.MakeStreamName("s_", "steps", name)
+        if err != nil {
+                panic(fmt.Errorf("failed to generate step name for [%s]: %s", as.Name(), err))
+        }
+
+        index := as.parent.logPathIndex[logPath]
+        as.parent.logPathIndex[logPath] = (index + 1)
+
+        // Append the index to the stream name.
+        logPath = logPath.Concat(types.StreamName(strconv.Itoa(index)))
+        if err := logPath.Validate(); err != nil {
+                panic(fmt.Errorf("generated invalid log stream path %q: %v", logPath, err))
+        }
+
+        as.LogNameBase = as.parent.BaseStream(logPath)
 }
 
 // Start marks the Step as started.
 func (as *Step) Start(startTime *google.Timestamp) bool {
         if as.Started != nil {
                 return false
         }
         as.Started = startTime
         return true
 }
@@ skipping 232 matching lines @@
         as.StderrStream, updated = as.maybeSetLogDogStream(as.StderrStream, st)
         return
 }
 
 func (as *Step) maybeSetLogDogStream(target *milo.LogdogStream, st *milo.LogdogStream) (*milo.LogdogStream, bool) {
         if (target == nil && st == nil) || (target != nil && st != nil && *target == *st) {
                 return target, false
         }
         return st, true
 }