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.

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

Logging: review once again #1639

Closed
antkmsft opened this issue Feb 9, 2021 · 1 comment · Fixed by #1752
Closed

Logging: review once again #1639

antkmsft opened this issue Feb 9, 2021 · 1 comment · Fixed by #1752
Assignees
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. design-discussion An area of design currently under discussion and open to team and community feedback. feature-request This issue requires a new behavior in the product in order be resolved.
Milestone

Comments

@antkmsft
Copy link
Member

antkmsft commented Feb 9, 2021

  • Default log level, what it should be - Verbose, or Warning, or something else?
  • Do we like free functions inside the Azure::Core::Logging:: namespace, or should we make a class?
  • std::function, or something else for callback?
  • Add string componentId? (other proposal was string serviceId) Or don't? Or not a string, but some enum?
  • LogLevels order - ascending or descending? - https://docs.microsoft.com/en-us/dotnet/api/microsoft.extensions.logging.loglevel?view=dotnet-plat-ext-5.0
  • Do we want more log levels? Any problems if we add more in the future?

Victor had cool proposal for the LoggingOptions, so logging callback+logLevel would behave something like HttpTransport - you don't have to specify it, or use different transport, but if you want, it is overridable.

  • Go and instrument more code with logging? One person does it, or everyone does their part? Storage does not currently have logging at all. Let's also come up with pattern, which log levels we use. Create a separate work item for that?

  • Use atomic instead of mutex? I'm pretty sure no one would disagree, but what if would require us to not use std::function in the user-facing API? (Logging: use atomics instead of mutex #1638)

(Just in case: #1568)

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Feb 9, 2021
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Feb 9, 2021
@antkmsft antkmsft added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Feb 9, 2021
@ahsonkhan
Copy link
Member

  • std::function, or something else for callback?

@CaseyCarter, I would love to hear your perspective on this. One of our goals with logging within the SDK is simplicity. We have quite a straightforward API set and model, which relies on the customer defining this callback method and registering it.
Is using std::function the best (and simplest or the customer) for such a callback?

typedef std::function<void(LogLevel level, std::string const& message)> LogListener;
/**
* @brief Set the function that will be invoked to report an SDK log message.
*
* @param logListener A #LogListener function that will be invoked when the SDK reports a log
* message matching one of the log levels passed to #SetLogLevel(). If `nullptr`, no
* function will be invoked.
*/
void SetLogListener(LogListener logListener);
/**
* @brief Sets the #LogLevel an application is interested in receiving.
*
* @param level Maximum log level.
*/
void SetLogLevel(LogLevel level);

@RickWinter RickWinter added Client This issue points to a problem in the data-plane of the library. design-discussion An area of design currently under discussion and open to team and community feedback. feature-request This issue requires a new behavior in the product in order be resolved. and removed needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. labels Feb 12, 2021
@RickWinter RickWinter added this to the [2021] March milestone Feb 12, 2021
@antkmsft antkmsft self-assigned this Feb 24, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. design-discussion An area of design currently under discussion and open to team and community feedback. feature-request This issue requires a new behavior in the product in order be resolved.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants