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

[Google Cloud Storage] Add logs #2552

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

[Google Cloud Storage] Add logs #2552

wants to merge 3 commits into from

Conversation

moxarth-elastic
Copy link
Collaborator

Part Of #2299

Adding more logs in GCS connector.

Log file: https://drive.google.com/file/d/1_ERs4rUGbEA54whCP6PZQZHLTmVmtFDl/view?usp=drive_link

Checklists

Pre-Review Checklist

  • this PR does NOT contain credentials of any kind, such as API keys or username/passwords (double check config.yml.example)
  • this PR has a meaningful title
  • this PR links to all relevant github issues that it fixes or partially addresses
  • if there is no GH issue, please create it. Each PR should have a link to an issue
  • this PR has a thorough description
  • Tested the changes locally
  • Added a label for each target release version (example: v7.13.2, v7.14.0, v8.0.0)

@moxarth-elastic moxarth-elastic requested a review from a team as a code owner May 14, 2024 10:13
@moxarth-elastic moxarth-elastic changed the title Add logging for GCS [Google Cloud Storage] Add logs May 14, 2024
@artem-shelkovnikov
Copy link
Member

From reading the logs:

  1. "Successfully connected to the Google Cloud Storage" should go to DEBUG. Failure should be a warning
  2. We need to have DEBUG logs that state which resource was called , e.g. "GET /users/123", can we add those?

Comment on lines +356 to +364
if not doit:
self._logger.debug(f"Skipping attachment downloading for {blob['name']}")
return

file_size = int(blob["size"])
if not (doit and file_size):
if file_size <= 0:
self._logger.warning(
f"Skipping file '{blob['name']}' as file size is {file_size}"
)
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch!

@moxarth-elastic
Copy link
Collaborator Author

buildkite test this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants