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

Removed old logging and implement Azure core logging coverage #18723

Merged
merged 1 commit into from
Nov 30, 2021
Merged

Removed old logging and implement Azure core logging coverage #18723

merged 1 commit into from
Nov 30, 2021

Conversation

jay-most
Copy link
Contributor

  • Removed old logging
  • Implement Azure core logging coverage for
    -ClientContext logging
    -ParallelQueryExecutionContextBase logging
    -RequestHandler logging

@ghost ghost added the Cosmos label Nov 17, 2021
@kirankumarkolli
Copy link
Member

@kushagraThapar , @moderakh , @simorenoh please take a look.

moderakh
moderakh reviewed Nov 18, 2021
View reviewed changes
sdk/cosmosdb/cosmos/src/queryExecutionContext/defaultQueryExecutionContext.ts Outdated
@@ -133,11 +131,11 @@ export class DefaultQueryExecutionContext implements ExecutionContext {
try {
let p: Promise<Response<any>>;
if (this.nextFetchFunction !== undefined) {
log.debug("using prefetch");
logger.info("Debug: using prefetch");
Copy link
Contributor

@moderakh moderakh Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

is it possible to only log certain log messages conditionally for debug level? meaning if the log level is not debug then we don't log certain messages?

doesn't azure logger have something like logger.debug that we can use here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me check what others are doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's automatic...any logs that are written using a log level equal to or less than the one you choose will be emitted.

Copy link
Contributor

@moderakh moderakh Nov 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

ogs that are written using a log level equal to or less than the one you choose will be

most logging framework work that way. slf4j/log4j in java work same way.

@jay-most what are the supported log levels by the azure-logger?

logger.info is too noisy for this. Is there a lower log level (less noisy) which can be used here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... log level equal to or less than the one you choose will be emitted. sounds like it will be chatty at any level above debug.
error, warning, verbose, info

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for explanation. In that case, if I understood correctly, it seems verbose is the closest to debug.

so as the old code was debug, isn't verbose more suitable here?

log.verbose("using prefetch");

I wonder if we can choose verbose here instead as it seems a more suitable replacement for debug?

@jay-most jay-most force-pushed the cosmosdb-implement-azurelogger branch from 79a6b01 to 03fd604 Compare November 19, 2021 15:41
@jay-most jay-most closed this Nov 19, 2021
@jay-most jay-most force-pushed the cosmosdb-implement-azurelogger branch from 03fd604 to 7f81c24 Compare November 19, 2021 22:35
@jay-most jay-most reopened this Nov 19, 2021
moderakh
moderakh approved these changes Nov 19, 2021
View reviewed changes
Copy link
Contributor

@moderakh moderakh left a comment

Choose a reason for hiding this comment

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

one minor comment regarding mapping of the old log levels to the new log levels. other than that LGTM. Thanks.

I think we should go with this:
debug -> verbose
info -> info
warn -> warning
error -> error

@jay-most jay-most force-pushed the cosmosdb-implement-azurelogger branch from e046ef7 to 93cef79 Compare November 19, 2021 23:13
simorenoh
simorenoh approved these changes Nov 22, 2021
View reviewed changes
Copy link
Member

@simorenoh simorenoh left a comment

Choose a reason for hiding this comment

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

Mo made a good point and seems like it's been fixed, lgtm!

@jay-most jay-most merged commit ba54673 into main Nov 30, 2021
@jay-most jay-most deleted the cosmosdb-implement-azurelogger branch November 30, 2021 21:19
@jay-most jay-most mentioned this pull request Aug 10, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weshaggard weshaggard weshaggard left review comments

@simorenoh simorenoh simorenoh approved these changes

@moderakh moderakh moderakh approved these changes

@kushagraThapar kushagraThapar Awaiting requested review from kushagraThapar

@simplynaveen20 simplynaveen20 Awaiting requested review from simplynaveen20

@xinlian12 xinlian12 Awaiting requested review from xinlian12

@danieljurek danieljurek Awaiting requested review from danieljurek

@mikeharder mikeharder Awaiting requested review from mikeharder

@praveenkuttappan praveenkuttappan Awaiting requested review from praveenkuttappan

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

Successfully merging this pull request may close these issues.
5 participants
@jay-most @kirankumarkolli @weshaggard @moderakh @simorenoh