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.
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
Less verbose logs on func start #2139
Conversation
This PR builds on previous PR: #2135 that forces whitelisted logs if Building on the same approach, running |
Is this related to #1131? |
Yes, I just linked the issue to the PR! |
@anthonychu - Please note that sample output has following log:
|
This is different that what we discussed in #1131 though. I think we need to discuss. |
Updated the PR, to turn off logs by default. @anthonychu - what is the experience we are aiming for function invocation logs? |
Just looked through the details: #1131 (comment)
|
@pragnagopa Sorry we didn't get a chance to sync up first before you started on this. @ankitkumarr and @fabiocav have more context, I can help catch you up. It's more than just setting logs to None. Logs coming from the host and Core Tools should be at Warning, and logs from customer code should remain at Information. I have updated this comment #1131 (comment) with the latest on what we landed on from the discussions over the past couple of months. While this covers multiple commands, let's scope this work to I have a proof of concept to demonstrate what we're looking for. This diff might be useful to explain some of the changes: 7dd097e...anthonychu:less-verbose Sample of what we're looking for is here: |
@pragnagopa Looks like you commented while I was typing up my reply. I think we're on the same page. 😄 |
7f85980
to
44886ab
Compare
c0a8b04
to
8146516
Compare
Updated PR description, added unit tests. Please review. |
ping for reviews! |
@pragnagopa One thing discussed in #1131 that got lost in the noise is defaulting to existing logging behavior in CI environments. Can check for this using something similar to this: https://github.com/watson/ci-info/blob/master/index.js#L52-L59 Other than that, it looks great based on the description. Thanks! |
f903a34
to
5ee3738
Compare
@anthonychu - not sure if there is a standard way to figure out CI environment. Added check for variables you pointed out: https://github.com/watson/ci-info/blob/master/index.js#L52-L59 We can expand the list in future if needed. |
@pragnagopa Thanks! We don’t need to catch them all, those variables should detect most of them. |
8fb72a4
to
1330607
Compare
|
||
public UserSecretsConfigurationBuilder(string scriptPath) | ||
public UserSecretsConfigurationBuilder(string scriptPath, LoggingFilterHelper loggingFilterHelper) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gzuber - can you please ACK these changes? Had to re-base and update this code so that all the logs go through default logging filters
4a80c5a
to
46ecbd1
Compare
@brettsam / @ankitkumarr - Green build! Can you please take final pass? |
.Callback(v => VerboseLogging = v); | ||
|
||
var parserResult = base.ParseArgs(args); | ||
bool verboseLoggingArgExists = parserResult.UnMatchedOptions.Where(o => o.LongName.Equals("verbose", StringComparison.OrdinalIgnoreCase)).Any(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably just do parserResult.UnMatchedOptions.Any(o => o.LongName.Equals("verbose", StringComparison.OrdinalIgnoreCase))
I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
Parser | ||
.Setup<bool>("verbose") | ||
.WithDescription("When false, hides system logs other than warnings and errors.") | ||
.SetDefault(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we remove the default value here, would we still need verboseLoggingArgExists
? We can probably just check if VerboseLogging
is null
or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parser is not binding properly to type bool?
. I tried updating the nuget package same behavior.
@anthonychu fyi - merging this PR. Will update the release notes to include this change. |
Fixes #2141 Fixes #2143 Fixes #2110
This PR:
--verbose
flag. Running with--verbose
flag will give users existing logging behavior--verbose
flag is set.func host start
will have less verbose system logs:Warning
Information
Default
log level is specified inhost.json
- both user logs and system logs level is set to the value specified inhost.json
host.json
has default loglevel set toNone
- Only whitelisted logs that VS Code uses will be loggedhost.json
lists any category then defaults do not applySample output on func host start with default host.json
Sample output on func host start with following host.json
NOTE: To see middleware tracing user explicitly needs to add that category in host.json
Output :