feat: add trace for executables #744

wangxiaoxuan273 · 2023-07-18T10:48:09Z

Resolves #662

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

Two-Hearts · 2023-07-19T12:55:51Z

internal/cmd/options.go

 	"github.com/notaryproject/notation/internal/trace"
+	executableTrace "github.com/oras-project/oras-credentials-go/trace"


When renaming an imported package, the local name should follow the same guidelines as package names (lower case, no under_scores or mixedCaps). (https://go.dev/blog/package-names)

Suggested change

executableTrace "github.com/oras-project/oras-credentials-go/trace"

executabletrace "github.com/oras-project/oras-credentials-go/trace"

How about calling it credentialstrace?

Changed the package name to credentialstrace.

Two-Hearts · 2023-07-19T12:59:59Z

internal/cmd/options.go

+			logger.Info("started executing credential helper program %s with action %s", executableName, action)
+		},
+		ExecuteDone: func(executableName, action string, err error) {
+			logger.Info("finished executing credential helper program %s with action %s and erro %v", executableName, action, err)


Suggested change

logger.Info("finished executing credential helper program %s with action %s and erro %v", executableName, action, err)

logger.Info("finished executing credential helper program %s with action %s and error %w", executableName, action, err)

changed accordingly.

internal/cmd/options.go

JeyJeyGao · 2023-07-19T13:21:39Z

internal/cmd/options.go

+	logger := log.GetLogger(ctx)
+	ctx = executableTrace.WithExecutableTrace(ctx, &executableTrace.ExecutableTrace{
+		ExecuteStart: func(executableName, action string) {
+			logger.Info("started executing credential helper program %s with action %s", executableName, action)


Should it be Info level or debug level? The process for accessing credential information is too detailed for a Notation user, and it is not the main logic of Notation.

changed to debug level.

internal/cmd/options.go

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

codecov-commenter · 2023-07-20T06:18:57Z

Codecov Report

Merging #744 (0e8c8d2) into main (354e74f) will increase coverage by 0.32%.
The diff coverage is 90.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #744      +/-   ##
==========================================
+ Coverage   63.66%   63.98%   +0.32%     
==========================================
  Files          40       40              
  Lines        2232     2252      +20     
==========================================
+ Hits         1421     1441      +20     
+ Misses        690      689       -1     
- Partials      121      122       +1

Impacted Files	Coverage Δ
cmd/notation/cert/show.go	`31.66% <0.00%> (ø)`
cmd/notation/inspect.go	`68.63% <ø> (ø)`
cmd/notation/list.go	`72.82% <ø> (ø)`
cmd/notation/login.go	`25.89% <ø> (ø)`
cmd/notation/key.go	`58.33% <66.66%> (ø)`
cmd/notation/cert/list.go	`52.43% <100.00%> (ø)`
cmd/notation/logout.go	`50.00% <100.00%> (ø)`
cmd/notation/sign.go	`79.38% <100.00%> (ø)`
cmd/notation/verify.go	`85.71% <100.00%> (ø)`
internal/cmd/options.go	`73.52% <100.00%> (-26.48%)`	⬇️

... and 28 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Wwwsylvia · 2023-07-20T06:44:09Z

internal/cmd/options.go

 	"github.com/notaryproject/notation/internal/trace"
+	executabletrace "github.com/oras-project/oras-credentials-go/trace"


I think the name executabletrace looks like it traces any kind of executables, while it just traces executables for credentials usage.

changed the package name to credentialstrace.

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

Wwwsylvia · 2023-07-20T07:34:18Z

go.sum

+github.com/oras-project/oras-credentials-go v0.2.1-0.20230714083315-2afb422fe225 h1:0T0CYijlSdn0ariTr48cRn6YDl445VZf3yqCqJKksqo=
+github.com/oras-project/oras-credentials-go v0.2.1-0.20230714083315-2afb422fe225/go.mod h1:fFCebDQo0Do+gnM96uV9YUnRay0pwuRQupypvofsp4s=


Cleanup is needed.

Ran go mod tidy.

internal/cmd/options.go

Wwwsylvia · 2023-07-20T07:52:38Z

internal/cmd/options.go

@@ -44,9 +46,27 @@ func (opts *LoggingFlagOpts) ApplyFlags(fs *pflag.FlagSet) {
 // SetLoggerLevel sets up the logger based on common options.
 func (opts *LoggingFlagOpts) SetLoggerLevel(ctx context.Context) context.Context {


A question: Do we need to rename this function given that it now does more than setting logger level? 🤔 @JeyJeyGao

Good point!

Renamed the function to InitializaLogger.

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

Two-Hearts

LGTM

JeyJeyGao

LGTM

Wwwsylvia

LGTM, but I'm not a maintainer.

shizhMSFT

LGTM

priteshbandi

LGTM

priteshbandi · 2023-07-21T05:20:46Z

internal/cmd/options.go

+			if err != nil {
+				logger.Errorf("finished executing credential helper program %s with action %s and got error %w", executableName, action, err)
+			} else {
+				logger.Debugf("finished executing credential helper program %s with action %s", executableName, action)


Suggested change

logger.Debugf("finished executing credential helper program %s with action %s", executableName, action)

logger.Debugf("successfully finished executing credential helper program %s with action %s", executableName, action)

changed accordingly

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

priteshbandi

LGTM

Two-Hearts

LGTM

shizhMSFT

LGTM

wangxiaoxuan273 requested review from gokarnm, JeyJeyGao, justincormack, NiazFK, priteshbandi, rgnote, shizhMSFT, SteveLasker and Two-Hearts as code owners July 18, 2023 10:48

wangxiaoxuan273 closed this Jul 19, 2023

feat: enable trace

56f56dd

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

wangxiaoxuan273 reopened this Jul 19, 2023

Two-Hearts reviewed Jul 19, 2023

View reviewed changes

JeyJeyGao reviewed Jul 19, 2023

View reviewed changes

internal/cmd/options.go Show resolved Hide resolved

wangxiaoxuan273 added 2 commits July 20, 2023 14:08

resolved comments

15a1efa

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

fixed a bug

03565ec

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

Wwwsylvia reviewed Jul 20, 2023

View reviewed changes

wangxiaoxuan273 added 2 commits July 20, 2023 14:49

updated oras-credentials-go version

0220eb3

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

renamed package

e8fe38a

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

Wwwsylvia reviewed Jul 20, 2023

View reviewed changes

JeyJeyGao reviewed Jul 20, 2023

View reviewed changes

internal/cmd/options.go Show resolved Hide resolved

Wwwsylvia reviewed Jul 20, 2023

View reviewed changes

resolved comments

32fcdaa

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

Two-Hearts previously approved these changes Jul 20, 2023

View reviewed changes

JeyJeyGao previously approved these changes Jul 20, 2023

View reviewed changes

Wwwsylvia approved these changes Jul 20, 2023

View reviewed changes

shizhMSFT previously approved these changes Jul 20, 2023

View reviewed changes

priteshbandi previously approved these changes Jul 21, 2023

View reviewed changes

priteshbandi reviewed Jul 21, 2023

View reviewed changes

added an adverb

0e8c8d2

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

wangxiaoxuan273 dismissed stale reviews from priteshbandi, shizhMSFT, JeyJeyGao, and Two-Hearts via 0e8c8d2 July 21, 2023 06:14

priteshbandi approved these changes Jul 21, 2023

View reviewed changes

Two-Hearts approved these changes Jul 21, 2023

View reviewed changes

shizhMSFT approved these changes Jul 21, 2023

View reviewed changes

shizhMSFT merged commit 161c972 into notaryproject:main Jul 21, 2023
5 checks passed

shizhMSFT mentioned this pull request Jul 31, 2023

bump: tag and release v1.0.0 #748

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add trace for executables #744

feat: add trace for executables #744

wangxiaoxuan273 commented Jul 18, 2023

Two-Hearts Jul 19, 2023

Wwwsylvia Jul 20, 2023

wangxiaoxuan273 Jul 20, 2023 •

edited

Two-Hearts Jul 19, 2023

wangxiaoxuan273 Jul 20, 2023

JeyJeyGao Jul 19, 2023

wangxiaoxuan273 Jul 20, 2023

codecov-commenter commented Jul 20, 2023 •

edited

Wwwsylvia Jul 20, 2023

wangxiaoxuan273 Jul 20, 2023

Wwwsylvia Jul 20, 2023 •

edited

wangxiaoxuan273 Jul 20, 2023

Wwwsylvia Jul 20, 2023

JeyJeyGao Jul 20, 2023

wangxiaoxuan273 Jul 20, 2023

Two-Hearts left a comment

JeyJeyGao left a comment

Wwwsylvia left a comment

shizhMSFT left a comment

priteshbandi left a comment

priteshbandi Jul 21, 2023

wangxiaoxuan273 Jul 21, 2023

priteshbandi left a comment

Two-Hearts left a comment

shizhMSFT left a comment

		"github.com/notaryproject/notation/internal/trace"
		executableTrace "github.com/oras-project/oras-credentials-go/trace"

	logger.Info("finished executing credential helper program %s with action %s and erro %v", executableName, action, err)
	logger.Info("finished executing credential helper program %s with action %s and error %w", executableName, action, err)

		github.com/oras-project/oras-credentials-go v0.2.1-0.20230714083315-2afb422fe225 h1:0T0CYijlSdn0ariTr48cRn6YDl445VZf3yqCqJKksqo=
		github.com/oras-project/oras-credentials-go v0.2.1-0.20230714083315-2afb422fe225/go.mod h1:fFCebDQo0Do+gnM96uV9YUnRay0pwuRQupypvofsp4s=

		@@ -44,9 +46,27 @@ func (opts LoggingFlagOpts) ApplyFlags(fs pflag.FlagSet) {
		// SetLoggerLevel sets up the logger based on common options.
		func (opts *LoggingFlagOpts) SetLoggerLevel(ctx context.Context) context.Context {

	logger.Debugf("finished executing credential helper program %s with action %s", executableName, action)
	logger.Debugf("successfully finished executing credential helper program %s with action %s", executableName, action)

feat: add trace for executables #744

feat: add trace for executables #744

Conversation

wangxiaoxuan273 commented Jul 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangxiaoxuan273 Jul 20, 2023 • edited

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

codecov-commenter commented Jul 20, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wwwsylvia Jul 20, 2023 • edited

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

Two-Hearts left a comment

Choose a reason for hiding this comment

JeyJeyGao left a comment

Choose a reason for hiding this comment

Wwwsylvia left a comment

Choose a reason for hiding this comment

shizhMSFT left a comment

Choose a reason for hiding this comment

priteshbandi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priteshbandi left a comment

Choose a reason for hiding this comment

Two-Hearts left a comment

Choose a reason for hiding this comment

shizhMSFT left a comment

Choose a reason for hiding this comment

wangxiaoxuan273 Jul 20, 2023 •

edited

codecov-commenter commented Jul 20, 2023 •

edited

Wwwsylvia Jul 20, 2023 •

edited