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

chore: add documentation for logging #2308

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

ohmayr
Copy link
Contributor

@ohmayr ohmayr commented Jan 6, 2025

Fixes b/384047953

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Jan 6, 2025
@ohmayr ohmayr marked this pull request as ready for review January 6, 2025 20:47
@ohmayr ohmayr requested a review from a team as a code owner January 6, 2025 20:47
Copy link
Contributor

@parthea parthea left a comment

Choose a reason for hiding this comment

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

Added minor observations, otherwise LGTM.

This library has support to enable logging for debugging and monitoring purposes. Note that logs may contain sensitive information and care should be
taken to restrict access to the logs if they are saved, whether it be on local storage or Google Cloud Logging.

Users must **explicitly opt-in** to enable logging by configuring the `GOOGLE_SDK_PYTHON_LOGGING_SCOPE` environment variable with a valid logging scope.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Users must **explicitly opt-in** to enable logging by configuring the `GOOGLE_SDK_PYTHON_LOGGING_SCOPE` environment variable with a valid logging scope.
By default this feature is not enabled. Users may opt-in to enable logging by configuring the `GOOGLE_SDK_PYTHON_LOGGING_SCOPE` environment variable with a valid logging scope.

- Valid logging scopes: :code:`google`, :code:`google.cloud.asset.v1`, :code:`google.api`, :code:`google.auth`, etc.
- Invalid logging scopes: :code:`foo`, :code:`123`, etc.

**NOTE**: If an invalid logging scope is configured, we do not act on the corresponding logger.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
**NOTE**: If an invalid logging scope is configured, we do not act on the corresponding logger.
**NOTE**: If an invalid logging scope is configured, debug logging will remain disabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • we shouldn't call this "debug logging" externally (see internal doc comment, for example)
  • "will remain disabled" is misleading, because the user could set up handlers a different way

I tried to clear this up in the suggested text.


export GOOGLE_SDK_PYTHON_LOGGING_SCOPE=google

To enable a handler for a specific GAPIC:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
To enable a handler for a specific GAPIC:
To enable a handler for a specific module:


.. code-block:: console

export GOOGLE_SDK_PYTHON_LOGGING_SCOPE=google.cloud.abc.v1
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export GOOGLE_SDK_PYTHON_LOGGING_SCOPE=google.cloud.abc.v1
export GOOGLE_SDK_PYTHON_LOGGING_SCOPE=google.cloud.abc_v1


export GOOGLE_SDK_PYTHON_LOGGING_SCOPE=google.cloud.abc.v1

:code:`GOOGLE_SDK_PYTHON_LOGGING_SCOPE` allows users to enable or disable logs by configuring a log system; it does not let them configure log levels, handlers, formatters, etc.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
:code:`GOOGLE_SDK_PYTHON_LOGGING_SCOPE` allows users to enable or disable logs by configuring a log system; it does not let them configure log levels, handlers, formatters, etc.
:code:`GOOGLE_SDK_PYTHON_LOGGING_SCOPE` allows users to enable or disable logs by configuring a log system; however it is not currently possible to customize log levels, handlers, formatters, etc.


:code:`GOOGLE_SDK_PYTHON_LOGGING_SCOPE` allows users to enable or disable logs by configuring a log system; it does not let them configure log levels, handlers, formatters, etc.

Loggers so configured will handle log messages at level DEBUG or higher, outputting them to stderr. The default log format for these log messages is structured log format.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Loggers so configured will handle log messages at level DEBUG or higher, outputting them to stderr. The default log format for these log messages is structured log format.
Configured loggers will handle log messages at level DEBUG or higher, outputting them to stderr. The default log format for these log messages is structured log format.


:code:`GOOGLE_SDK_PYTHON_LOGGING_SCOPE` allows users to enable or disable logs by configuring a log system; it does not let them configure log levels, handlers, formatters, etc.

Loggers so configured will handle log messages at level DEBUG or higher, outputting them to stderr. The default log format for these log messages is structured log format.
Copy link
Contributor

@parthea parthea Jan 7, 2025

Choose a reason for hiding this comment

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

Would it be helpful to provide a one line sample of the log output so that users can see what debug logs will look like when logging is configured?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how much value that adds. Plus, since we're giving ourselves the leeway to change the log messages, this could easily become stale, depending on how much information we provide here.

Copy link
Contributor

@vchudnov-g vchudnov-g Jan 8, 2025

Choose a reason for hiding this comment

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

I found it easier to make changes separately, and this is the text I suggest. WDYT?

Some notes:

  1. If you prefer to discuss the text in a Google Doc first, I can easily export it to one and we can discuss the content there.
  2. It would be great to not use a generic example library in this file (in the content below, google.cloud.library_v1) but rather the actual name of the library we're generating. This should be simple because we're in a template, but if it's not, let's file a follow-up issue to get back to this.

(sorry I couldn't do this directly in RST. Once we agree on the content, I should be able to export to RST with minimal set-up on my part, if that's helpful.)

SUGGESTED CONTENT FOLLOWS


Logging

This library uses the standard Python logging functionality to log some RPC events that could be of interest for debugging and monitoring purposes. Note the following:

  1. Logs may contain sensitive information. Take care to restrict access to the logs if they are saved, whether it be on local storage or Google Cloud Logging.
  2. Google may refine the occurrence, level, and content of various log messages in this library without flagging such changes as breaking. Do not depend on immutability of the logging events.
  3. By default, the logging events from this library are not handled. You must explicitly configure log handling using one of the mechanisms below.

Simple, environment-based configuration

To enable logging for this library without any changes in your code, set the GOOGLE_SDK_PYTHON_LOGGING_SCOPE environment variable to a valid Google logging scope. This configures handling of logging events (at level logging.DEBUG or higher) from this library in a default manner, emitting the logged messages in a structured format. It does not currently allow customizing the logging levels captured nor the handlers, formatters, etc. used for any logging event.

A logging scope is a period-separated namespace that begins with google, identifying the Python module or package to log.

  • Valid logging scopes: google, google.cloud.asset.v1, google.api, google.auth, etc.
  • Invalid logging scopes: foo, 123, etc.

NOTE: If the logging scope is invalid, the library does not set up any logging handlers.

Examples

  • Enabling the default handler for all Google-based loggers

    export GOOGLE_SDK_PYTHON_LOGGING_SCOPE=google
    
  • Enabling the default handler for a specific Google module

    In the case of a client library called Library:

    export GOOGLE_SDK_PYTHON_LOGGING_SCOPE=google.cloud.library_v1
    

Advanced, code-based configuration

You can also configure a valid logging scope using Python's standard logging mechanism.

Examples

  • Configuring a handler for all Google-based loggers

    import logging
    
    from google.cloud.translate_v3 import translate
    
    base_logger = logging.getLogger("google")
    base_logger.addHandler(logging.StreamHandler())
    base_logger.setLevel(logging.DEBUG)
    
  • Configuring a handler for a specific Google module

    In the case of a client library called Library:

    import logging
    
    from google.cloud.translate_v3 import translate
    
    base_logger = logging.getLogger("google.cloud.library_v1")
    base_logger.addHandler(logging.StreamHandler())
    base_logger.setLevel(logging.DEBUG)
    

Logging details

  1. Regardless of which of the mechanisms above you use to configure logging for this library, by default logging events are not propagated up to the root logger from the google-level logger. If you need the events to be propagated to the root logger, you must explicitly set logging.getLogger("google").propagate = True in your code.
  2. You can mix the different logging configurations above for different Google modules. For example, you may want use a code-based logging configuration for one library, but decide you need to also set up environment-based logging configuration for another library.
    1. If you attempt to use both code-based and environment-based configuration for the same module, the environment-based configuration will be ineffectual if the code -based configuration gets applied first.
  3. The Google-specific logging configurations (default handlers for environment-based configuration; not propagating logging events to the root logger) get executed the first time any client library is instantiated in your application, and only if the affected loggers have not been previously configured. (This is the reason for 2.1 above.)

LOGGING
-------

This library has support to enable logging for debugging and monitoring purposes. Note that logs may contain sensitive information and care should be
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it's clear or accurate to say we support "enabling" logging. We log, in the sense we emit logging events. Users can set up handlers for those events, either explicitly via code or with the help of our environment-based config, which we describe below. I tried to clear this up in the suggested text.

@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Jan 9, 2025
@ohmayr ohmayr force-pushed the add-readme-for-logging branch from bca5207 to 70a7646 Compare January 14, 2025 16:17
Copy link
Contributor

@vchudnov-g vchudnov-g left a comment

Choose a reason for hiding this comment

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

If it's not trivial to use the info for the library being generated rather than the generic "library" API, could you file an issue to follow up on that? Thanks! (This was suggestion 2 in my previous comment)

gapic/templates/README.rst.j2 Outdated Show resolved Hide resolved
gapic/templates/README.rst.j2 Outdated Show resolved Hide resolved
gapic/templates/README.rst.j2 Outdated Show resolved Hide resolved
:code:`logging.getLogger("google").propagate = True` in your code.
#. You can mix the different logging configurations above for different Google modules. For example, you may want use a code-based logging configuration for
one library, but decide you need to also set up environment-based logging configuration for another library.
1. If you attempt to use both code-based and environment-based configuration for the same module, the environment-based configuration will be ineffectual
Copy link
Contributor

@vchudnov-g vchudnov-g Jan 22, 2025

Choose a reason for hiding this comment

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

This nested 1 does not render correctly (at least according to the GitHub preview if you click on the page icon, the second from the left, in the set of icons for this file shown right above this diff, on the right).
icon

We need to specify it's a sub-bullet.

@ohmayr
Copy link
Contributor Author

ohmayr commented Jan 23, 2025

If it's not trivial to use the info for the library being generated rather than the generic "library" API, could you file an issue to follow up on that? Thanks! (This was suggestion 2 in my previous comment)

Filed #2314

Copy link
Contributor

@vchudnov-g vchudnov-g left a comment

Choose a reason for hiding this comment

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

Just one minor change for consistency.

Thanks so much for doing this, Omair! It looks great!

gapic/templates/README.rst.j2 Outdated Show resolved Hide resolved
Co-authored-by: Victor Chudnovsky <[email protected]>
@ohmayr ohmayr enabled auto-merge (squash) January 24, 2025 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants