-
Notifications
You must be signed in to change notification settings - Fork 140
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
base: main
Are you sure you want to change the base?
[Dropbox ] Add logs #2555
Conversation
buildkite test this |
@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 |
There was a problem hiding this 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.
- 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) - The debug logs have inconsistent wording (they use a mix of
Fetching ...
orRetrieving ...
). Let's useFetching
for API calls andRetrieving
for other operations (like.get
against a dict) - The logfile you provided doesn't contain any logs from this PR. Was it on the correct branch when you ran it?
- 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?
connectors/sources/dropbox.py
Outdated
self._logger.debug( | ||
f"Token expiration time '{self.token_expiration_time}' is not in the correct format. Converting it into ISO format" | ||
) |
There was a problem hiding this comment.
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.
connectors/sources/dropbox.py
Outdated
@@ -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") |
There was a problem hiding this comment.
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.
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 |
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
config.yml.example
)v7.13.2
,v7.14.0
,v8.0.0
)