-
Notifications
You must be signed in to change notification settings - Fork 78
Synapse Profiler Scripts #2020
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
Synapse Profiler Scripts #2020
Conversation
|
✅ 39/39 passed, 5 flaky, 2m12s total Flaky tests:
Running from acceptance #2633 |
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 need a better interface to execute
I am not sure if we want to leave it to the developers to always initiate, the workspace and creds. this will lead to inconsistencies sooner or later.
I also would split extracting the "metrics" and persisting them so we need at least two methods extract and persist
src/databricks/labs/lakebridge/resources/assessments/synapse/common/connector.py
Outdated
Show resolved
Hide resolved
src/databricks/labs/lakebridge/resources/assessments/synapse/common/connector.py
Show resolved
Hide resolved
src/databricks/labs/lakebridge/resources/assessments/synapse/common/functions.py
Outdated
Show resolved
Hide resolved
src/databricks/labs/lakebridge/resources/assessments/synapse/common/functions.py
Outdated
Show resolved
Hide resolved
src/databricks/labs/lakebridge/resources/assessments/synapse/common/functions.py
Outdated
Show resolved
Hide resolved
src/databricks/labs/lakebridge/resources/assessments/synapse/dedicated_sqlpool_extract.py
Show resolved
Hide resolved
src/databricks/labs/lakebridge/resources/assessments/synapse/monitoring_metrics_extract.py
Show resolved
Hide resolved
src/databricks/labs/lakebridge/resources/assessments/synapse/workspace_extract.py
Show resolved
Hide resolved
src/databricks/labs/lakebridge/resources/assessments/synapse/serverless_sqlpool_extract.py
Show resolved
Hide resolved
I have simplified the usage in the latest push The primary purpose of this module is to retrieve information from system tables using queries and metrics using APIs, categorised into the following four brackets:
I don't believe further separating extract and persist makes sense. |
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 good
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.
LGTM! 🚢
| conn = duckdb.connect(db_path) | ||
|
|
||
| # Drop existing table if it exists | ||
| conn.execute(f"DROP TABLE IF EXISTS {table_name}") |
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.
If the profiler is run again the next day, for example, there is no way to accumulate history, correct?
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.
Seems that there are 2 similar functions for inserting profiler data, but radically different behavior. Workspace artifacts and monitoring metrics leverage this function, which does not support an append. However, serverless SQL pools and dedicated SQL pools leverage a function called save_resultset_to_db, which does support and append. Why the difference in behavior for these sets of Synapse objects? Perhaps combine the functions into a single function and use the mode param to dictate the behavior?
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 will address this in followup PR. Good catch.
| Enhanced version of save_resultset_to_db with predetermined schemas. | ||
| """ | ||
|
|
||
| # Predetermined schemas |
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.
+1
Changes
What does this PR do?
Relevant implementation details
Caveats/things to watch out for when reviewing:
Linked issues
Resolves #..
Functionality
databricks labs lakebridge ...Tests