Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Normal logs should not go to stderr (#3491) #5574

Closed
wants to merge 1 commit into from
Closed

fix: Normal logs should not go to stderr (#3491) #5574

wants to merge 1 commit into from

Conversation

iam-veeramalla
Copy link
Member

@iam-veeramalla iam-veeramalla commented Feb 22, 2021 •
edited
Loading

Signed-off-by: iam-veeramalla [email protected]

This PR fixes #3491. Send high level logs to Stderr and logs of normal execution to Stdout.
This PR sends Info, Debug, Warn and Trace logs to stdout. Error, Panic and Fatal are sent to stderr.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

@jannfis jannfis self-requested a review February 22, 2021 17:11
mnacharov
mnacharov suggested changes Feb 23, 2021
View reviewed changes
Copy link

@mnacharov mnacharov left a comment

Choose a reason for hiding this comment

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

Hi!
I think it will be better to apply new logging logic to argocd-dex too but keep cli (argocd and argocd-util) not touched

Also, it looks like e2e tests will not fail if cli will keep sending logs to stderr

cmd/argocd/commands/root.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 17, 2021 •
edited
Loading

Codecov Report

Merging #5574 (57637c0) into master (9c849e6) will increase coverage by 0.12%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5574      +/-   ##
==========================================
+ Coverage   40.88%   41.01%   +0.12%     
==========================================
  Files         146      146              
  Lines       19571    19612      +41     
==========================================
+ Hits         8002     8043      +41     
  Misses      10455    10455              
  Partials     1114     1114
Impacted Files Coverage Δ
util/env/env.go 78.78% <0.00%> (-10.87%) ⬇️
server/server.go 54.82% <0.00%> (-0.29%) ⬇️
reposerver/repository/repository.go 62.46% <0.00%> (+0.05%) ⬆️
util/oidc/oidc.go 19.02% <0.00%> (+0.36%) ⬆️
util/jwt/jwt.go 61.90% <0.00%> (+0.61%) ⬆️
util/tls/tls.go 79.42% <0.00%> (+4.73%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c849e6...57637c0. Read the comment docs.

@iam-veeramalla
Copy link
Member Author

@jannfis @mnacharov Can you have a relook at this ?

I moved away from the logrus hooks approach as it was duplicating every log and degrading the performance. I wrote a simple wrapper as suggested in the logrus community by the maintainers.

However, we have to figure out a new package if this change does not seem very efficient.

@iam-veeramalla iam-veeramalla mentioned this pull request Mar 17, 2021
Open
mnacharov
mnacharov suggested changes Mar 17, 2021
View reviewed changes
Copy link

@mnacharov mnacharov left a comment

Choose a reason for hiding this comment

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

Looks fine, but I would suggest some tweaks

util/cli/cli.go
// SetLogOutput sets log output to different streams
func SetLogOutput() {
log.SetOutput(&LogWriter{})
}
Copy link

Choose a reason for hiding this comment

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

I think it would be better to initialize levelRegex in SetLogOutput function instead of init

util/cli/cli.go
func (w *LogWriter) Write(p []byte) (n int, err error) {
level := w.detectLogLevel(p)

if level == LevelError || level == LevelWarning || level == LevelFatal || level == LevelPanic {
Copy link

Choose a reason for hiding this comment

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

I don't see profit of "Level*" variables since there is no way to customize logic of Writer

what do you think of this implementation?

levelRegex, err = regexp.Compile("level=(panic|fatal|warning|error)")
// ...
if levelRegex.Match(p) {
  return os.Stderr.Write(p)
}
return os.Stderr.Write(p)

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense 🙂

@iam-veeramalla iam-veeramalla mentioned this pull request Apr 28, 2021
Closed
@iam-veeramalla
Copy link
Member Author

iam-veeramalla commented Jun 1, 2021 •
edited
Loading

I will perform some performance testing on this PR to analyze the impact.

@lirlia
Copy link
Contributor

lirlia commented Nov 23, 2021

this pr look good to me,
how's going?

@pasha-codefresh
Copy link
Member

@iam-veeramalla could you please update on this?

@iam-veeramalla
Copy link
Member Author

Hi @pasha-codefresh , We need to move out of the existing logging package as it is in maintianence mode. This PR does not solve the problem completely.

We need to identify the package that we want to move to and re-work on this PR.

@pasha-codefresh
Copy link
Member

can we close this pr for now and reopen new one with improved solution, so we will not have stale PR? @iam-veeramalla

@iam-veeramalla
Copy link
Member Author

closing this PR, will open a new one with more details.

@crenshaw-dev crenshaw-dev mentioned this pull request Jul 26, 2022
Closed
@bob-bbb
Copy link

bob-bbb commented Oct 26, 2022 •
edited
Loading

@iam-veeramalla
how's going?
i want to use fixed version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnacharov mnacharov mnacharov requested changes

@jannfis jannfis Awaiting requested review from jannfis

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Normal logs should not go to stderr
5 participants
@iam-veeramalla @lirlia @pasha-codefresh @bob-bbb @mnacharov