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.

Sign up for GitHub

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(webdriver): Tuning the Webdriver logging a bit #23255

Merged
merged 1 commit into from
Mar 1, 2023
Merged

chore(webdriver): Tuning the Webdriver logging a bit #23255

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 10 additions & 11 deletions superset/utils/webdriver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,9 @@ def find_unexpected_errors(driver: WebDriver) -> List[str]:
f"arguments[0].innerHTML = '{error_as_html}'", alert_div
)
except WebDriverException:
logger.warning(
"Failed to update error messages using alert_div", exc_info=True
)
logger.exception("Failed to update error messages using alert_div")
except WebDriverException:
logger.warning("Failed to capture unexpected errors", exc_info=True)
logger.exception("Failed to capture unexpected errors")

return error_messages

Expand Down Expand Up @@ -142,7 +140,7 @@ def create(self) -> WebDriver:
options.add_argument(arg)

kwargs.update(current_app.config["WEBDRIVER_CONFIGURATION"])
logger.info("Init selenium driver")
logger.debug("Init selenium driver")

return driver_class(**kwargs)

Expand Down Expand Up @@ -200,7 +198,7 @@ def get_screenshot(
]
logger.debug("Wait %i seconds for chart animation", selenium_animation_wait)
sleep(selenium_animation_wait)
logger.info(
logger.debug(
Copy link
Member

Choose a reason for hiding this comment

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

@craig-rueda I left a question for you about this on my PR, to see if it's safe to keep as an info: https://github.com/apache/superset/pull/23114/files#diff-ee5af2b5ab4555c2f1d3d31d44fecd1c14bf03e13aa88cb0d15862d7dcea57bbL220

"Taking a PNG screenshot of url %s as user %s",
url,
user.username,
Expand All @@ -219,15 +217,16 @@ def get_screenshot(
img = element.screenshot_as_png

except TimeoutException:
logger.warning("Selenium timed out requesting url %s", url, exc_info=True)
logger.exception("Selenium timed out requesting url %s", url)
Copy link
Member

Choose a reason for hiding this comment

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

This is going to give us a lot of errors in the application that aren't necessarily system errors, but could be a result of a user error or an issue with a user's database performance. Are we sure we want to log these as exceptions? I'm currently seeing the stack trace under exc_info in the logs?

Copy link
Member Author

Choose a reason for hiding this comment

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

I see what you're saying, but it's my opinion that exceptions should be logged as such. If there's timeouts, we should adjust the timeout value in order to prevent this. If there's some network issue, etc. we should look into that as well.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense. I think some of these could be user-related, as in the user's dataset is broken. Maybe we can split out the errors to get a better sense of what's going on, and separate out the system errors from the user errors. I put together a PR here.. lmk what you think. #23290

except StaleElementReferenceException:
logger.error(
logger.exception(
"Selenium got a stale element while requesting url %s",
url,
exc_info=True,
)
except WebDriverException as ex:
logger.error(ex, exc_info=True)
except WebDriverException:
logger.exception(
"Encountered an unexpected error when requeating url %s", url
)
finally:
self.destroy(driver, current_app.config["SCREENSHOT_SELENIUM_RETRIES"])
return img