Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Oct 11, 2024

Currently the debug log has issues under multi-thread scenarios. This may cause a request log being followed by a response log of another request, which is confusing for log readers.

This PR fixes the issue by labeling the request and response pairs.

This PR does not change any log content. It only makes the log more readable.

Patrick Zheng added 4 commits October 11, 2024 09:01
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
@codecov
Copy link

codecov bot commented Oct 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.02%. Comparing base (6f2860a) to head (fe71e87).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1061      +/-   ##
==========================================
+ Coverage   72.00%   72.02%   +0.02%     
==========================================
  Files          46       46              
  Lines        2300     2302       +2     
==========================================
+ Hits         1656     1658       +2     
  Misses        442      442              
  Partials      202      202              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Patrick Zheng <[email protected]>
@ghost ghost requested a review from JeyJeyGao October 11, 2024 05:02
Signed-off-by: Patrick Zheng <[email protected]>
JeyJeyGao
JeyJeyGao previously approved these changes Oct 11, 2024
Copy link
Contributor

@JeyJeyGao JeyJeyGao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@JeyJeyGao
Copy link
Contributor

As suggested by Shiwei, ORAS's implementation may be better.

Signed-off-by: Patrick Zheng <[email protected]>
logHeader(req.Header, e)
// log the request
e.Debugf("Request #%d\n> Request: %q %q\n> Request headers:\n%s",
id, req.Method, req.URL, logHeader(req.Header))

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information

[Sensitive data returned by HTTP request headers](1) flows to a logging call.
@ghost ghost requested review from JeyJeyGao and shizhMSFT October 11, 2024 07:05
JeyJeyGao
JeyJeyGao previously approved these changes Oct 11, 2024
Copy link
Contributor

@JeyJeyGao JeyJeyGao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Signed-off-by: Patrick Zheng <[email protected]>
@ghost ghost requested review from JeyJeyGao and shizhMSFT October 14, 2024 06:50
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ghost ghost merged commit fa81e79 into notaryproject:main Oct 14, 2024
@ghost ghost deleted the log branch October 14, 2024 23:59
7h3-3mp7y-m4n pushed a commit to 7h3-3mp7y-m4n/notation that referenced this pull request Mar 29, 2025
FeynmanZhou pushed a commit to FeynmanZhou/notation that referenced this pull request May 15, 2025
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants