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

[Dropbox ] Add logs #2555

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

[Dropbox ] Add logs #2555

wants to merge 2 commits into from

Conversation

moxarth-elastic
Copy link
Collaborator

@moxarth-elastic moxarth-elastic commented May 14, 2024

Part Of #2299

Adding more logs in Dropbox connector.

Log file: https://drive.google.com/file/d/1njpVpExhM5Fw_efUgvl9SRxSGOtr-UAA/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
Copy link
Collaborator Author

buildkite test this

@navarone-feekery
Copy link
Contributor

@moxarth-elastic can you provide the debug log output for this during a standard run as requested on the issue?

@moxarth-elastic
Copy link
Collaborator Author

@moxarth-elastic can you provide the debug log output for this during a standard run as requested on the issue?

Sure, attached a log file here: https://drive.google.com/file/d/1njpVpExhM5Fw_efUgvl9SRxSGOtr-UAA/view?usp=drive_link

Copy link
Contributor

@navarone-feekery navarone-feekery left a comment

Choose a reason for hiding this comment

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

Nice! I have some change requests.

  1. I think a log should also be added to map_permission_with_document (single debug log with keys of docs that will have permissions mapped to them)
  2. The debug logs have inconsistent wording (they use a mix of Fetching ... or Retrieving ...). Let's use Fetching for API calls and Retrieving for other operations (like .get against a dict)
  3. The logfile you provided doesn't contain any logs from this PR. Was it on the correct branch when you ran it?
  4. Also half of the new debug logs are for ACL, but the provided logfile was for a regular sync. Can you also link a logfile for an ACL sync?

Comment on lines 192 to 194
self._logger.debug(
f"Token expiration time '{self.token_expiration_time}' is not in the correct format. Converting it into ISO format"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know that this log provides much value. It isn't a significant code fork path and it isn't changing data being synced, it's just fixing a var type. I think it can be removed.

@@ -987,6 +1000,7 @@ def get_email(self, permission, identity):
async def get_permission(self, permission, account_id):
permissions = []
if identities := permission.get("users"):
self._logger.debug("Fetching users")
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 unclear on what exactly is being logged here. There are no API calls happening to fetch users. Also there are no logs for invitees or groups. I think this log is unnecessary.

@navarone-feekery
Copy link
Contributor

I've also just been made aware that a similar PR is out, so I think we should include the missing logs from that PR into this one: #2324

@navarone-feekery navarone-feekery self-assigned this May 27, 2024
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