-
Notifications
You must be signed in to change notification settings - Fork 148
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 more logging #2324
Conversation
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.
Doesn't look like we typically crawl 1 folder at a time, but in DLS we do. Can you add an INFO log in async for folder_id in self.get_team_folder_id():
to share progress on which folder we're at?
@@ -316,6 +316,8 @@ async def api_call(self, base_url, url_name, data=None, file_type=None, **kwargs | |||
headers = self._get_request_headers( | |||
file_type=file_type, url_name=url_name, kwargs=kwargs | |||
) | |||
|
|||
self._logger.debug(f"Calling Dropbox Endpoint: {url}") |
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 suggest adding the headers, since the dropbox API is weird and uses headers for their params
Co-authored-by: Sean Story <sean.j.story@gmail.com>
Co-authored-by: Sean Story <sean.j.story@gmail.com>
Co-authored-by: Chenhui Wang <54903978+wangch079@users.noreply.github.com>
connectors/sources/dropbox.py
Outdated
@@ -316,6 +316,10 @@ async def api_call(self, base_url, url_name, data=None, file_type=None, **kwargs | |||
headers = self._get_request_headers( | |||
file_type=file_type, url_name=url_name, kwargs=kwargs | |||
) | |||
|
|||
self._logger.debug( | |||
f"Calling Dropbox Endpoint: {url} with headers: {headers}" |
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.
just double-checking: don't we expose API keys when we log "headers"?
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.
Looks like it yes: https://github.com/elastic/connectors/blob/main/connectors/sources/dropbox.py#L243-L245. Good catch! Then I'll remove the headers again
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.
We could also remove the auth header explicitly before logging headers, but still feels a little bit dangerous to be honest.
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 didn't think about auth headers at all when I made that suggestion. Good catch.
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.
What about just logging X-Dropbox*
headers? (I think that's the prefix?)
Can you include example logs before/after for this connector too, to help reviewing it better? |
Bump |
Closing this PR; @navarone-feekery will make sure that useful logs will be added to https://github.com/elastic/connectors/pull/2555/files. Thanks Nav! |
Adding some more logging to the dropbox connector to log API calls and some progress indicators.
Checklists
Pre-Review Checklist
- [ ] 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(Simple 5 minute change improving logs)
v7.13.2
,v7.14.0
,v8.0.0
)- [] if you added or changed Rich Configurable Fields for a Native Connector, you made a corresponding PR in Kibana