Skip to content

Change the Infof to Debugf in KindMap() to fix #3835.#3925

Merged
yangminzhu merged 1 commit intoistio:masterfrom
yangminzhu:fixLogOutput
Mar 3, 2018
Merged

Change the Infof to Debugf in KindMap() to fix #3835.#3925
yangminzhu merged 1 commit intoistio:masterfrom
yangminzhu:fixLogOutput

Conversation

@yangminzhu
Copy link
Copy Markdown
Contributor

In #3759, it changed pkg/log/log.go to use default logging options which sets to InfoLevel by default.

@yangminzhu yangminzhu requested review from a team, geeknoid, mandarjog and wattli March 3, 2018 01:14
@yangminzhu
Copy link
Copy Markdown
Contributor Author

I'm trying to pick some starter issues to help to ramp up on Istio, please let me know if you think we should not change the default logging level, I'll try to fix the issue in a different way.
Thank you!

@wattli
Copy link
Copy Markdown
Contributor

wattli commented Mar 3, 2018

/lgtm, but i will defer the approval to Mandar/Martin

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 3, 2018

Codecov Report

Merging #3925 into master will increase coverage by 1%.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3925   +/-   ##
======================================
+ Coverage      76%     76%   +1%     
======================================
  Files         297     297           
  Lines       27210   27197   -13     
======================================
+ Hits        20537   20578   +41     
+ Misses       5348    5291   -57     
- Partials     1325    1328    +3
Impacted Files Coverage Δ
mixer/pkg/runtime2/config/kinds.go 100% <100%> (ø) ⬆️
mixer/pkg/runtime/init.go 28% <100%> (ø) ⬆️
mixer/adapter/stackdriver/helper/common.go 63% <0%> (-15%) ⬇️
mixer/adapter/stackdriver/stackdriver.go 55% <0%> (-15%) ⬇️
.../adapter/solarwinds/internal/appoptics/batching.go 70% <0%> (-8%) ⬇️
mixer/adapter/servicecontrol/distValueBuilder.go 87% <0%> (-4%) ⬇️
mixer/adapter/statsd/statsd.go 96% <0%> (-1%) ⬇️
mixer/adapter/servicecontrol/utils.go 89% <0%> (ø) ⬇️
mixer/adapter/kubernetesenv/kubernetesenv.go 68% <0%> (ø) ⬇️
mixer/adapter/memquota/keys.go 100% <0%> (ø) ⬆️
... and 9 more

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 afd5911...aae4079. Read the comment docs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can't change this for the product, the intent is for Info to be the default and changing this now will remove a considerable amount of log info we depend on.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So I guess until we have a better solution in #2400, we won't be able to have a simple fix for this #3835, any ideas or should I close this?

@geeknoid
Copy link
Copy Markdown
Contributor

geeknoid commented Mar 3, 2018

This change is not appropriate, the default log level must stay at Info for the product.

What specific issue is this trying to fix?

@ldemailly
Copy link
Copy Markdown
Member

#2400

@yangminzhu
Copy link
Copy Markdown
Contributor Author

This is to fix #3835.

@ldemailly
Copy link
Copy Markdown
Member

@yangminzhu the proper fix is probably to move the spew to Debug rather than change the default to Warning. That or decide because it's a one time spew it is actually appropriate and useful to see at startup/once

What we want to make sure is we don't log anything at Info for every request.

@geeknoid
Copy link
Copy Markdown
Contributor

geeknoid commented Mar 3, 2018

The solution to #3835 is definitely just to change the simple log statement in mixs and not to change the system's default log level globally.

Thanks.

@yangminzhu
Copy link
Copy Markdown
Contributor Author

@geeknoid Thanks for the suggestion!

These logging information are generated from the calling to runtime.KindMap() in mixer/cmd/mixs/cmd/validator.go:39. And it's KindMap() that calls log.Infof() which in turn produces these information. (ea7303f#diff-9476892d25087e7c68a6c9343c536b64R72)

@mandarjog Could you let me know if it's ok to change the log.Infof() in KindMap() to Debug level to fix the issue?
Thanks!

@geeknoid
Copy link
Copy Markdown
Contributor

geeknoid commented Mar 3, 2018 via email

@yangminzhu yangminzhu changed the title Change the default log level from InfoLevel to WarnLevel to fix #3835. Change the Infof to Debugf in KindMap() to fix #3835. Mar 3, 2018
@geeknoid
Copy link
Copy Markdown
Contributor

geeknoid commented Mar 3, 2018

/approve

@geeknoid
Copy link
Copy Markdown
Contributor

geeknoid commented Mar 3, 2018

/lgtm

@istio-merge-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: geeknoid

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@istio-merge-robot
Copy link
Copy Markdown

/lgtm cancel //PR changed after LGTM, removing LGTM. @geeknoid @yangminzhu

@istio-testing
Copy link
Copy Markdown
Collaborator

@yangminzhu: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
prow/istio-presubmit.sh aae4079 link /test istio-presubmit
Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@yangminzhu yangminzhu merged commit f434fc2 into istio:master Mar 3, 2018
@yangminzhu yangminzhu deleted the fixLogOutput branch March 6, 2018 21:32
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.

7 participants