feat: add trace for executables#744
feat: add trace for executables#744shizhMSFT merged 7 commits intonotaryproject:mainfrom wangxiaoxuan273:enable-trace
Conversation
Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>
internal/cmd/options.go
Outdated
|
|
||
| "github.com/notaryproject/notation-go/log" | ||
| "github.com/notaryproject/notation/internal/trace" | ||
| executableTrace "github.com/oras-project/oras-credentials-go/trace" |
There was a problem hiding this comment.
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)
| executableTrace "github.com/oras-project/oras-credentials-go/trace" | |
| executabletrace "github.com/oras-project/oras-credentials-go/trace" |
There was a problem hiding this comment.
How about calling it credentialstrace?
There was a problem hiding this comment.
Changed the package name to credentialstrace.
internal/cmd/options.go
Outdated
| 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) |
There was a problem hiding this comment.
| 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) |
There was a problem hiding this comment.
changed accordingly.
internal/cmd/options.go
Outdated
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
changed to debug level.
Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>
Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>
Codecov Report
❗ 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
... and 28 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
internal/cmd/options.go
Outdated
|
|
||
| "github.com/notaryproject/notation-go/log" | ||
| "github.com/notaryproject/notation/internal/trace" | ||
| executabletrace "github.com/oras-project/oras-credentials-go/trace" |
There was a problem hiding this comment.
I think the name executabletrace looks like it traces any kind of executables, while it just traces executables for credentials usage.
There was a problem hiding this comment.
changed the package name to credentialstrace.
Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>
Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>
go.sum
Outdated
| 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= |
There was a problem hiding this comment.
Ran go mod tidy.
internal/cmd/options.go
Outdated
| @@ -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 { | |||
There was a problem hiding this comment.
A question: Do we need to rename this function given that it now does more than setting logger level? 🤔 @JeyJeyGao
There was a problem hiding this comment.
Renamed the function to InitializaLogger.
Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>
Wwwsylvia
left a comment
There was a problem hiding this comment.
LGTM, but I'm not a maintainer.
internal/cmd/options.go
Outdated
| 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) |
There was a problem hiding this comment.
| 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) |
There was a problem hiding this comment.
changed accordingly
Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>
0e8c8d2
Resolves notaryproject#662 --------- Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>
Resolves notaryproject#662 --------- Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>
Resolves #662