Enhance the trace related code

* Move request id to requestid middleware
* fix span pass to child ctx on orm
* fix typos
* remove unused code
* add operation name to Transaction

Signed-off-by: Qian Deng <dengq@vmware.com>

make/harbor.yml.tmpl

@@ -209,7 +209,7 @@ proxy:
 #   enabled: true
 #   # set sample_rate to 1 if you wanna sampling 100% of trace data; set 0.5 if you wanna sampling 50% of trace data, and so forth
 #   sample_rate: 1
-#   # # namespace used to diferenciate different harbor services
+#   # # namespace used to differenciate different harbor services
 #   # namespace:
 #   # # attributes is a key value dict contains user defined attributes used to initialize trace provider
 #   # attributes:

make/photon/prepare/migrations/version_2_4_0/harbor.yml.jinja

@@ -494,7 +494,7 @@ trace:
 #   enabled: true
 #   # set sample_rate to 1 if you wanna sampling 100% of trace data; set 0.5 if you wanna sampling 50% of trace data, and so forth
 #   sample_rate: 1
-#   # # namespace used to diferenciate different harbor services
+#   # # namespace used to differenciate different harbor services
 #   # namespace:
 #   # # attributes is a key value dict contains user defined attributes used to initialize trace provider
 #   # attributes:

src/controller/artifact/controller.go

@@ -204,7 +204,7 @@ func (c *controller) ensureArtifact(ctx context.Context, repository, digest stri
 		created = true
 		artifact.ID = id
 		return nil
-	})(ctx); err != nil {
+	})(orm.SetTransactionOpNameToContext(ctx, "tx-ensure-artifact")); err != nil {
 		// got error that isn't conflict error, return directly
 		if !errors.IsConflictErr(err) {
 			return false, nil, err
@@ -376,7 +376,7 @@ func (c *controller) deleteDeeply(ctx context.Context, id int64, isRoot bool) er
 			Digest:            art.Digest,
 		})
 		return err
-	})(ctx); err != nil && !errors.IsErr(err, errors.ConflictCode) {
+	})(orm.SetTransactionOpNameToContext(ctx, "tx-delete-artifact-deeply")); err != nil && !errors.IsErr(err, errors.ConflictCode) {
 		return err
 	}
 

src/controller/blob/controller.go

@@ -17,9 +17,10 @@ package blob
 import (
 	"context"
 	"fmt"
-	"github.com/goharbor/harbor/src/lib/q"
 	"time"
 
+	"github.com/goharbor/harbor/src/lib/q"
+
 	"github.com/docker/distribution"
 	"github.com/goharbor/harbor/src/lib/errors"
 	"github.com/goharbor/harbor/src/lib/log"
@@ -303,7 +304,7 @@ func (c *controller) Sync(ctx context.Context, references []distribution.Descrip
 			}
 
 			return nil
-		})(ctx)
+		})(orm.SetTransactionOpNameToContext(ctx, "tx-sync-blob"))
 	}
 
 	if len(missing) > 0 {

src/controller/event/handler/internal/util.go

@@ -41,5 +41,5 @@ func autoScan(ctx context.Context, a *artifact.Artifact, tags ...string) error {
 		}
 
 		return scan.DefaultController.Scan(ctx, a, options...)
-	})(ctx)
+	})(orm.SetTransactionOpNameToContext(ctx, "tx-auto-scan"))
 }

src/controller/project/controller.go

@@ -104,7 +104,7 @@ func (c *controller) Create(ctx context.Context, project *models.Project) (int64
 		return nil
 	}
 
-	if err := orm.WithTransaction(h)(ctx); err != nil {
+	if err := orm.WithTransaction(h)(orm.SetTransactionOpNameToContext(ctx, "tx-create-project")); err != nil {
 		return 0, err
 	}
 

src/controller/project/controller_test.go

@@ -50,16 +50,16 @@ func (suite *ControllerTestSuite) TestCreate() {
 	c := controller{projectMgr: mgr, allowlistMgr: allowlistMgr, metaMgr: metadataMgr}
 
 	{
-		metadataMgr.On("Add", ctx, mock.Anything, mock.Anything).Return(nil).Once()
-		mgr.On("Create", ctx, mock.Anything).Return(int64(2), nil).Once()
+		metadataMgr.On("Add", mock.Anything, mock.Anything, mock.Anything).Return(nil).Once()
+		mgr.On("Create", mock.Anything, mock.Anything).Return(int64(2), nil).Once()
 		projectID, err := c.Create(ctx, &models.Project{OwnerID: 1, Metadata: map[string]string{"public": "true"}})
 		suite.Nil(err)
 		suite.Equal(int64(2), projectID)
 	}
 
 	{
-		metadataMgr.On("Add", ctx, mock.Anything, mock.Anything).Return(fmt.Errorf("oops")).Once()
-		mgr.On("Create", ctx, mock.Anything).Return(int64(2), nil).Once()
+		metadataMgr.On("Add", mock.Anything, mock.Anything, mock.Anything).Return(fmt.Errorf("oops")).Once()
+		mgr.On("Create", mock.Anything, mock.Anything).Return(int64(2), nil).Once()
 		projectID, err := c.Create(ctx, &models.Project{OwnerID: 1, Metadata: map[string]string{"public": "true"}})
 		suite.Error(err)
 		suite.Equal(int64(0), projectID)

src/controller/repository/controller.go

@@ -108,7 +108,7 @@ func (c *controller) Ensure(ctx context.Context, name string) (bool, int64, erro
 		}
 		created = true
 		return nil
-	})(ctx); err != nil {
+	})(orm.SetTransactionOpNameToContext(ctx, "tx-repository-ensure")); err != nil {
 		// isn't conflict error, return directly
 		if !errors.IsConflictErr(err) {
 			return false, 0, err

src/controller/scan/base_controller.go

@@ -379,7 +379,7 @@ func (bc *basicController) startScanAll(ctx context.Context, executionID int64)
 			return bc.Scan(ctx, artifact, WithExecutionID(executionID))
 		}
 
-		if err := orm.WithTransaction(scan)(bc.makeCtx()); err != nil {
+		if err := orm.WithTransaction(scan)(orm.SetTransactionOpNameToContext(bc.makeCtx(), "tx-start-scanall")); err != nil {
 			// Just logged
 			log.Errorf("failed to scan artifact %s, error %v", artifact, err)
 
@@ -500,7 +500,7 @@ func (bc *basicController) makeReportPlaceholder(ctx context.Context, r *scanner
 			return nil
 		}
 
-		if err := orm.WithTransaction(create)(ctx); err != nil {
+		if err := orm.WithTransaction(create)(orm.SetTransactionOpNameToContext(ctx, "tx-make-report-placeholder")); err != nil {
 			return nil, err
 		}
 

src/controller/tag/controller.go

@@ -16,6 +16,8 @@ package tag
 
 import (
 	"context"
+	"time"
+
 	"github.com/goharbor/harbor/src/common/utils"
 	"github.com/goharbor/harbor/src/lib/errors"
 	"github.com/goharbor/harbor/src/lib/log"
@@ -28,7 +30,6 @@ import (
 	"github.com/goharbor/harbor/src/pkg/signature"
 	"github.com/goharbor/harbor/src/pkg/tag"
 	model_tag "github.com/goharbor/harbor/src/pkg/tag/model/tag"
-	"time"
 )
 
 var (
@@ -115,7 +116,7 @@ func (c *controller) Ensure(ctx context.Context, repositoryID, artifactID int64,
 		tag.PushTime = time.Now()
 		_, err = c.Create(ctx, tag)
 		return err
-	})(ctx); err != nil && !errors.IsConflictErr(err) {
+	})(orm.SetTransactionOpNameToContext(ctx, "tx-tag-ensure")); err != nil && !errors.IsConflictErr(err) {
 		return err
 	}
 

src/jobservice/runner/redis.go

@@ -92,7 +92,7 @@ func (rj *RedisJob) Run(j *work.Job) (err error) {
 				b := backoff(retried)
 				logger.Errorf("Track job %s: stats may not have been ready yet, hold for %d ms and retry again", jID, b)
 				<-time.After(time.Duration(b) * time.Millisecond)
-				span.AddEvent("retring to get job stat")
+				span.AddEvent("retrying to get job stat")
 				continue
 			} else {
 				// Exit and never try.

src/lib/config/metadata/value.go

@@ -95,6 +95,7 @@ func (c *ConfigureValue) GetInt64() int64 {
 	return 0
 }
 
+// GetFloat64 - return the float64 value of current value
 func (c *ConfigureValue) GetFloat64() float64 {
 	if item, ok := Instance().GetByName(c.Name); ok {
 		val, err := item.ItemType.get(c.Value)

src/lib/config/metadata/value_test.go

@@ -16,8 +16,9 @@ package metadata
 
 import (
 	"fmt"
-	"github.com/stretchr/testify/assert"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 var testingMetaDataArray = []Item{
@@ -27,6 +28,7 @@ var testingMetaDataArray = []Item{
 	{Name: "ldap_verify_cert", ItemType: &BoolType{}, Scope: "user", Group: "ldapbasic"},
 	{Name: "sample_map_setting", ItemType: &MapType{}, Scope: "user", Group: "ldapbasic"},
 	{Name: "scan_all_policy", ItemType: &MapType{}, Scope: "user", Group: "basic"},
+	{Name: "sample_rate", ItemType: &Float64Type{}, Scope: "system", Group: "basic"},
 }
 
 // createCfgValue ... Create a ConfigureValue object, only used in test
@@ -67,6 +69,11 @@ func TestConfigureValue_GetInt64(t *testing.T) {
 	assert.Equal(t, createCfgValue("ulimit", "99999").GetInt64(), int64(99999))
 }
 
+func TestConfigureValue_GetFloat64(t *testing.T) {
+	Instance().initFromArray(testingMetaDataArray)
+	assert.Equal(t, createCfgValue("sample_rate", "0.5").GetFloat64(), float64(0.5))
+}
+
 func TestNewScanAllPolicy(t *testing.T) {
 	Instance().initFromArray(testingMetaDataArray)
 	value, err := NewCfgValue("scan_all_policy", `{"parameter":{"daily_time":0},"type":"daily"}`)

src/lib/orm/orm.go

@@ -50,7 +50,10 @@ func RegisterModel(models ...interface{}) {
 
 type ormKey struct{}
 
-const tracerName = "goharbor/harbor/src/lib/orm"
+const (
+	tracerName               = "goharbor/harbor/src/lib/orm"
+	defaultTranscationOpName = "start-transaction"
+)
 
 func init() {
 	if os.Getenv("ORM_DEBUG") == "true" {
@@ -85,10 +88,32 @@ func Clone(ctx context.Context) context.Context {
 	return NewContext(ctx, orm.NewOrm())
 }
 
+type operationNameKey struct{}
+
+// SetTransactionOpName sets the transaction operation name
+func SetTransactionOpNameToContext(ctx context.Context, name string) context.Context {
+	if ctx == nil {
+		ctx = context.Background()
+	}
+	return context.WithValue(ctx, operationNameKey{}, name)
+}
+
+// GetTransactionOpNameFromContext returns the transaction operation name from context
+func GetTransactionOpNameFromContext(ctx context.Context) string {
+	opName, ok := ctx.Value(operationNameKey{}).(string)
+	if !ok {
+		return defaultTranscationOpName
+	}
+	if opName == "" {
+		return defaultTranscationOpName
+	}
+	return opName
+}
+
 // WithTransaction a decorator which make f run in transaction
 func WithTransaction(f func(ctx context.Context) error) func(ctx context.Context) error {
 	return func(ctx context.Context) error {
-		_, span := tracelib.StartTrace(ctx, tracerName, "start-transaction")
+		cx, span := tracelib.StartTrace(ctx, tracerName, GetTransactionOpNameFromContext(ctx))
 		defer span.End()
 		o, err := FromContext(ctx)
 		if err != nil {
@@ -103,7 +128,7 @@ func WithTransaction(f func(ctx context.Context) error) func(ctx context.Context
 			return err
 		}
 
-		if err := f(ctx); err != nil {
+		if err := f(cx); err != nil {
 			span.AddEvent("rollback transaction")
 			if e := tx.Rollback(); e != nil {
 				tracelib.RecordError(span, e, "rollback transaction failed")

src/lib/trace/trace.go

@@ -72,7 +72,7 @@ func initExporter(ctx context.Context) (tracesdk.SpanExporter, error) {
 	return exp, err
 }
 
-func initProvider(exp tracesdk.SpanExporter) (*tracesdk.TracerProvider, error) {
+func initProvider(exp tracesdk.SpanExporter) *tracesdk.TracerProvider {
 	cfg := GetGlobalConfig()
 
 	// prepare attribute resources
@@ -102,8 +102,7 @@ func initProvider(exp tracesdk.SpanExporter) (*tracesdk.TracerProvider, error) {
 		tracesdk.WithSampler(tracesdk.TraceIDRatioBased(cfg.SampleRate)),
 	)
 	// init trace provider
-	tp := tracesdk.NewTracerProvider(ops...)
-	return tp, nil
+	return tracesdk.NewTracerProvider(ops...)
 }
 
 // ShutdownFunc is a function to shutdown the trace provider
@@ -122,8 +121,7 @@ func InitGlobalTracer(ctx context.Context) ShutdownFunc {
 	}
 	exp, err := initExporter(ctx)
 	handleErr(err, "fail in exporter initialization")
-	tp, err := initProvider(exp)
-	handleErr(err, "fail in tracer initialization")
+	tp := initProvider(exp)
 	otel.SetTracerProvider(tp)
 	otel.SetTextMapPropagator(propagation.NewCompositeTextMapPropagator(propagation.TraceContext{}, propagation.Baggage{}))
 	return func() {

src/pkg/p2p/preheat/dao/instance/dao.go

@@ -106,7 +106,7 @@ func (d *dao) Update(ctx context.Context, instance *provider.Instance, props ...
 		_, err = o.Update(instance, props...)
 		return
 	}
-	return orm.WithTransaction(trans)(ctx)
+	return orm.WithTransaction(trans)(orm.SetTransactionOpNameToContext(ctx, "tx-prehead-update"))
 
 }
 

src/pkg/project/dao/dao.go

@@ -86,7 +86,7 @@ func (d *dao) Create(ctx context.Context, project *models.Project) (int64, error
 		return nil
 	}
 
-	if err := orm.WithTransaction(h)(ctx); err != nil {
+	if err := orm.WithTransaction(h)(orm.SetTransactionOpNameToContext(ctx, "tx-create-project")); err != nil {
 		return 0, err
 	}
 

src/pkg/project/metadata/manager.go

@@ -65,7 +65,7 @@ func (m *manager) Add(ctx context.Context, projectID int64, meta map[string]stri
 		}
 		return nil
 	}
-	return orm.WithTransaction(h)(ctx)
+	return orm.WithTransaction(h)(orm.SetTransactionOpNameToContext(ctx, "tx-add-project"))
 }
 
 // Delete metadatas whose keys are specified in parameter meta, if it is absent, delete all
@@ -89,7 +89,7 @@ func (m *manager) Update(ctx context.Context, projectID int64, meta map[string]s
 		return nil
 	}
 
-	return orm.WithTransaction(h)(ctx)
+	return orm.WithTransaction(h)(orm.SetTransactionOpNameToContext(ctx, "tx-delete-project"))
 }
 
 // Get metadatas whose keys are specified in parameter meta, if it is absent, get all

src/pkg/quota/manager.go

@@ -74,7 +74,7 @@ func (m *manager) Create(ctx context.Context, reference, referenceID string, har
 		return err
 	}
 
-	err = orm.WithTransaction(h)(ctx)
+	err = orm.WithTransaction(h)(orm.SetTransactionOpNameToContext(ctx, "tx-create-quota"))
 
 	return id, err
 }
@@ -88,7 +88,7 @@ func (m *manager) Delete(ctx context.Context, id int64) error {
 		return m.dao.Delete(ctx, id)
 	}
 
-	return orm.WithTransaction(h)(ctx)
+	return orm.WithTransaction(h)(orm.SetTransactionOpNameToContext(ctx, "tx-delete-quota"))
 }
 
 func (m *manager) Get(ctx context.Context, id int64) (*Quota, error) {

src/pkg/scan/dao/scan/vulnerability.go

@@ -140,7 +140,7 @@ func (v *vulnerabilityRecordDao) InsertForReport(ctx context.Context, reportUUID
 		return err
 	}
 
-	if err := orm.WithTransaction(h)(ctx); err != nil {
+	if err := orm.WithTransaction(h)(orm.SetTransactionOpNameToContext(ctx, "tx-insert-for-report")); err != nil {
 		fields := log.Fields{
 			"error":  err,
 			"report": reportUUID,

src/pkg/scan/dao/scanner/registration.go

@@ -167,7 +167,7 @@ func SetDefaultRegistration(ctx context.Context, UUID string) error {
 		return err
 	}
 
-	return orm.WithTransaction(f)(ctx)
+	return orm.WithTransaction(f)(orm.SetTransactionOpNameToContext(ctx, "tx-scan-set-default-registration"))
 }
 
 // GetDefaultRegistration gets the default registration

src/registryctl/api/registry/manifest/manifest.go

@@ -16,7 +16,7 @@ import (
 	"go.opentelemetry.io/otel/trace"
 )
 
-const tracerName = "goharbor/harbor/src/registryctl/api/registry/blob"
+const tracerName = "goharbor/harbor/src/registryctl/api/registry/manifest"
 
 // NewHandler returns the handler to handler manifest request
 func NewHandler(storageDriver storagedriver.StorageDriver) http.Handler {
@@ -71,7 +71,7 @@ func (h *handler) delete(w http.ResponseWriter, r *http.Request) {
 	var tags []string
 	cleaner := storage.NewVacuum(ctx, h.storageDriver)
 	if err := cleaner.RemoveManifest(repoName, dgst, tags); err != nil {
-		tracelib.RecordError(span, err, "failed to remove blob")
+		tracelib.RecordError(span, err, "failed to remove manifest")
 		log.Infof("failed to remove manifest: %s, with error:%v", ref, err)
 		api.HandleError(w, err)
 		return

src/server/middleware/blob/put_blob_upload.go

@@ -78,7 +78,7 @@ func PutBlobUploadMiddleware() func(http.Handler) http.Handler {
 			return nil
 		}
 
-		return orm.WithTransaction(h)(ctx)
+		return orm.WithTransaction(h)(orm.SetTransactionOpNameToContext(ctx, "tx-put-blob-mw"))
 	})
 
 	return middleware.Chain(before, after)

src/server/middleware/log/log.go

@@ -17,11 +17,7 @@ package log
 import (
 	"net/http"
 
-	"go.opentelemetry.io/otel/attribute"
-	oteltrace "go.opentelemetry.io/otel/trace"
-
 	"github.com/goharbor/harbor/src/lib/log"
-	tracelib "github.com/goharbor/harbor/src/lib/trace"
 	"github.com/goharbor/harbor/src/server/middleware"
 )
 
@@ -34,9 +30,6 @@ func Middleware() func(http.Handler) http.Handler {
 			logger.Debugf("attach request id %s to the logger for the request %s %s", rid, r.Method, r.URL.Path)
 
 			ctx := log.WithLogger(r.Context(), logger.WithFields(log.Fields{"requestID": rid}))
-			if tracelib.Enabled() {
-				oteltrace.SpanFromContext(ctx).SetAttributes(attribute.Key("request-id").String(rid))
-			}
 			next.ServeHTTP(w, r.WithContext(ctx))
 		} else {
 			next.ServeHTTP(w, r)

src/server/middleware/requestid/requestid.go

@@ -17,7 +17,10 @@ package requestid
 import (
 	"net/http"
 
+	tracelib "github.com/goharbor/harbor/src/lib/trace"
 	"github.com/goharbor/harbor/src/server/middleware"
+	"go.opentelemetry.io/otel/attribute"
+	oteltrace "go.opentelemetry.io/otel/trace"
 
 	"github.com/google/uuid"
 )
@@ -34,6 +37,9 @@ func Middleware(skippers ...middleware.Skipper) func(http.Handler) http.Handler
 			r.Header.Set(HeaderXRequestID, rid)
 		}
 		w.Header().Set(HeaderXRequestID, rid)
+		if tracelib.Enabled() {
+			oteltrace.SpanFromContext(r.Context()).SetAttributes(attribute.Key(HeaderXRequestID).String(rid))
+		}
 		next.ServeHTTP(w, r)
 	}, skippers...)
 }