From 354a2bd80d9efc1bcefe48ccac06394af62de556 Mon Sep 17 00:00:00 2001 From: Qian Deng Date: Fri, 17 Sep 2021 16:07:24 +0000 Subject: [PATCH] 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 --- make/harbor.yml.tmpl | 2 +- .../migrations/version_2_4_0/harbor.yml.jinja | 2 +- src/controller/artifact/controller.go | 4 +-- src/controller/blob/controller.go | 5 +-- src/controller/event/handler/internal/util.go | 2 +- src/controller/project/controller.go | 2 +- src/controller/project/controller_test.go | 8 ++--- src/controller/repository/controller.go | 2 +- src/controller/scan/base_controller.go | 4 +-- src/controller/tag/controller.go | 5 +-- src/jobservice/runner/redis.go | 2 +- src/lib/config/metadata/value.go | 1 + src/lib/config/metadata/value_test.go | 9 +++++- src/lib/orm/orm.go | 31 +++++++++++++++++-- src/lib/trace/trace.go | 8 ++--- src/pkg/p2p/preheat/dao/instance/dao.go | 2 +- src/pkg/project/dao/dao.go | 2 +- src/pkg/project/metadata/manager.go | 4 +-- src/pkg/quota/manager.go | 4 +-- src/pkg/scan/dao/scan/vulnerability.go | 2 +- src/pkg/scan/dao/scanner/registration.go | 2 +- .../api/registry/manifest/manifest.go | 4 +-- src/server/middleware/blob/put_blob_upload.go | 2 +- src/server/middleware/log/log.go | 7 ----- src/server/middleware/requestid/requestid.go | 6 ++++ 25 files changed, 77 insertions(+), 45 deletions(-) diff --git a/make/harbor.yml.tmpl b/make/harbor.yml.tmpl index 3472bd32a..2470960dd 100644 --- a/make/harbor.yml.tmpl +++ b/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: diff --git a/make/photon/prepare/migrations/version_2_4_0/harbor.yml.jinja b/make/photon/prepare/migrations/version_2_4_0/harbor.yml.jinja index 0093290fd..ac5393ef1 100644 --- a/make/photon/prepare/migrations/version_2_4_0/harbor.yml.jinja +++ b/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: diff --git a/src/controller/artifact/controller.go b/src/controller/artifact/controller.go index 1b294c92d..590bb5c4b 100644 --- a/src/controller/artifact/controller.go +++ b/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 } diff --git a/src/controller/blob/controller.go b/src/controller/blob/controller.go index 08c03d081..e82dccff9 100644 --- a/src/controller/blob/controller.go +++ b/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 { diff --git a/src/controller/event/handler/internal/util.go b/src/controller/event/handler/internal/util.go index 5a5e1b110..c7cf51243 100644 --- a/src/controller/event/handler/internal/util.go +++ b/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")) } diff --git a/src/controller/project/controller.go b/src/controller/project/controller.go index f49bf33df..ade14a6ea 100644 --- a/src/controller/project/controller.go +++ b/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 } diff --git a/src/controller/project/controller_test.go b/src/controller/project/controller_test.go index a81cbcc89..e344b8ddc 100644 --- a/src/controller/project/controller_test.go +++ b/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) diff --git a/src/controller/repository/controller.go b/src/controller/repository/controller.go index f68f27e18..81cb46d89 100644 --- a/src/controller/repository/controller.go +++ b/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 diff --git a/src/controller/scan/base_controller.go b/src/controller/scan/base_controller.go index 99a8f6f7b..f6b902ea3 100644 --- a/src/controller/scan/base_controller.go +++ b/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 } diff --git a/src/controller/tag/controller.go b/src/controller/tag/controller.go index a878b626e..5bbe857c8 100644 --- a/src/controller/tag/controller.go +++ b/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 } diff --git a/src/jobservice/runner/redis.go b/src/jobservice/runner/redis.go index 5b1247f60..c6a6686fa 100644 --- a/src/jobservice/runner/redis.go +++ b/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. diff --git a/src/lib/config/metadata/value.go b/src/lib/config/metadata/value.go index 68e00a79b..ad9d738aa 100644 --- a/src/lib/config/metadata/value.go +++ b/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) diff --git a/src/lib/config/metadata/value_test.go b/src/lib/config/metadata/value_test.go index 8afea2c05..a28bfc56d 100644 --- a/src/lib/config/metadata/value_test.go +++ b/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"}`) diff --git a/src/lib/orm/orm.go b/src/lib/orm/orm.go index 8d7202e0f..f3ef18cb1 100644 --- a/src/lib/orm/orm.go +++ b/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") diff --git a/src/lib/trace/trace.go b/src/lib/trace/trace.go index 9d561b863..c6ce23745 100644 --- a/src/lib/trace/trace.go +++ b/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() { diff --git a/src/pkg/p2p/preheat/dao/instance/dao.go b/src/pkg/p2p/preheat/dao/instance/dao.go index caa411c40..5fcef42e4 100644 --- a/src/pkg/p2p/preheat/dao/instance/dao.go +++ b/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")) } diff --git a/src/pkg/project/dao/dao.go b/src/pkg/project/dao/dao.go index 68339240b..29eca1b5a 100644 --- a/src/pkg/project/dao/dao.go +++ b/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 } diff --git a/src/pkg/project/metadata/manager.go b/src/pkg/project/metadata/manager.go index ac43acf57..7d1274eb6 100644 --- a/src/pkg/project/metadata/manager.go +++ b/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 diff --git a/src/pkg/quota/manager.go b/src/pkg/quota/manager.go index 28d58322e..e2a3ca901 100644 --- a/src/pkg/quota/manager.go +++ b/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) { diff --git a/src/pkg/scan/dao/scan/vulnerability.go b/src/pkg/scan/dao/scan/vulnerability.go index 41f027729..da59cabe0 100644 --- a/src/pkg/scan/dao/scan/vulnerability.go +++ b/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, diff --git a/src/pkg/scan/dao/scanner/registration.go b/src/pkg/scan/dao/scanner/registration.go index 41f29e77d..4b1190e0b 100644 --- a/src/pkg/scan/dao/scanner/registration.go +++ b/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 diff --git a/src/registryctl/api/registry/manifest/manifest.go b/src/registryctl/api/registry/manifest/manifest.go index 000a44e21..f221d2388 100644 --- a/src/registryctl/api/registry/manifest/manifest.go +++ b/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 diff --git a/src/server/middleware/blob/put_blob_upload.go b/src/server/middleware/blob/put_blob_upload.go index 4e630e3a2..f12af221a 100644 --- a/src/server/middleware/blob/put_blob_upload.go +++ b/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) diff --git a/src/server/middleware/log/log.go b/src/server/middleware/log/log.go index 9240c5b56..715745636 100644 --- a/src/server/middleware/log/log.go +++ b/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) diff --git a/src/server/middleware/requestid/requestid.go b/src/server/middleware/requestid/requestid.go index a3c11e348..5fac4e05c 100644 --- a/src/server/middleware/requestid/requestid.go +++ b/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...) }