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

Add support for Lakehouse monitoring in bundles #1307

Merged
merged 22 commits into from
May 31, 2024

Conversation

aravind-segu
Copy link
Contributor

@aravind-segu aravind-segu commented Mar 24, 2024

Changes

This change adds support for Lakehouse monitoring in bundles.

The associated resource type name is "quality monitor".

Testing

Unit tests.

@pietern pietern changed the title [MLOPS-651] Support Lakehouse monitoring in Bundles Support Lakehouse monitoring in bundles Mar 25, 2024
bundle/internal/tf/schema/resource_lakehouse_monitor.go Outdated Show resolved Hide resolved
bundle/config/resources.go Outdated Show resolved Hide resolved
bundle/schema/openapi.go Outdated Show resolved Hide resolved
libs/dyn/convert/normalize.go Outdated Show resolved Hide resolved
libs/dyn/convert/normalize.go Outdated Show resolved Hide resolved
libs/dyn/convert/struct_info.go Outdated Show resolved Hide resolved
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

Thanks for the patch. Please reach out directly if you want to chat about the approach.

@aravind-segu
Copy link
Contributor Author

Regarding the hacky approach This looks pretty hacky. Can't we use the url tag of the field?. The url field is empty as well. CreateMonitor takes in the table name as part of the query parameters and it is not passed in json body.

@aravind-segu
Copy link
Contributor Author

Tests are failing because of the change to not avoid json - parameters in struct_info. How can we solve this? Is it ok to modify the tests?

Copy link
Contributor

@andrewnester andrewnester left a comment

Choose a reason for hiding this comment

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

FullName needs to be renamed to TableName because it was renamed in a new GoSDK version being used

bundle/deploy/terraform/convert_test.go Outdated Show resolved Hide resolved
bundle/deploy/terraform/convert_test.go Outdated Show resolved Hide resolved
bundle/tests/lakehouse_monitor_test.go Outdated Show resolved Hide resolved
libs/dyn/convert/struct_info.go Outdated Show resolved Hide resolved
bundle/deploy/terraform/tfdyn/convert_lakehouse_monitor.go Outdated Show resolved Hide resolved
@aravind-segu aravind-segu force-pushed the lakehouseMonitoring branch from a915754 to cfe77aa Compare May 6, 2024 03:08
@codecov-commenter
Copy link

codecov-commenter commented May 6, 2024

Codecov Report

Attention: Patch coverage is 51.25000% with 39 lines in your changes are missing coverage. Please review.

Project coverage is 53.58%. Comparing base (e22dd8a) to head (a2384c1).
Report is 124 commits behind head on main.

Files Patch % Lines
bundle/config/resources.go 0.00% 15 Missing and 3 partials ⚠️
libs/dyn/convert/struct_info.go 0.00% 6 Missing and 1 partial ⚠️
bundle/deploy/terraform/convert.go 70.00% 5 Missing and 1 partial ⚠️
.../deploy/terraform/tfdyn/convert_quality_monitor.go 64.70% 4 Missing and 2 partials ⚠️
bundle/deploy/terraform/interpolate.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1307      +/-   ##
==========================================
+ Coverage   52.25%   53.58%   +1.33%     
==========================================
  Files         317      351      +34     
  Lines       18004    20268    +2264     
==========================================
+ Hits         9408    10861    +1453     
- Misses       7903     8609     +706     
- Partials      693      798     +105     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aravind-segu aravind-segu requested a review from andrewnester May 6, 2024 03:16
@arpitjasa-db arpitjasa-db requested a review from pietern May 6, 2024 20:33
bundle/config/mutator/process_target_mode_test.go Outdated Show resolved Hide resolved
bundle/config/mutator/run_as_test.go Outdated Show resolved Hide resolved
bundle/deploy/terraform/convert_test.go Outdated Show resolved Hide resolved
bundle/schema/openapi.go Outdated Show resolved Hide resolved
bundle/tests/monitor/databricks.yml Outdated Show resolved Hide resolved
libs/dyn/convert/struct_info.go Outdated Show resolved Hide resolved
@aravind-segu aravind-segu force-pushed the lakehouseMonitoring branch from 8d4a1a9 to c240c31 Compare May 25, 2024 00:19
bundle/config/mutator/process_target_mode_test.go Outdated Show resolved Hide resolved
bundle/config/resources/quality_monitor.go Outdated Show resolved Hide resolved
bundle/config/resources/quality_monitor.go Outdated Show resolved Hide resolved
bundle/deploy/terraform/convert_test.go Show resolved Hide resolved
bundle/schema/openapi.go Outdated Show resolved Hide resolved
libs/dyn/convert/struct_info.go Outdated Show resolved Hide resolved
@pietern
Copy link
Contributor

pietern commented May 31, 2024

The TF bump is happening in #1460.

@pietern pietern changed the title Support Lakehouse monitoring in bundles Add support for Lakehouse monitoring in bundles May 31, 2024
Copy link
Contributor

@andrewnester andrewnester left a comment

Choose a reason for hiding this comment

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

Approved with a minor comment

libs/dyn/convert/struct_info.go Outdated Show resolved Hide resolved
@pietern pietern enabled auto-merge May 31, 2024 09:37
@pietern pietern disabled auto-merge May 31, 2024 09:37
@pietern pietern enabled auto-merge May 31, 2024 09:40
@pietern pietern added this pull request to the merge queue May 31, 2024
Merged via the queue into databricks:main with commit a33d0c8 May 31, 2024
5 checks passed
@arpitjasa-db arpitjasa-db deleted the lakehouseMonitoring branch May 31, 2024 18:37
pietern added a commit that referenced this pull request Jun 4, 2024
CLI:
 * Update OpenAPI spec ([#1466](#1466)).

Bundles:
 * Upgrade TF provider to 1.46.0 ([#1460](#1460)).
 * Add support for Lakehouse monitoring ([#1307](#1307)).
 * Make dbt-sql and default-sql templates public ([#1463](#1463)).

Internal:
 * Abstract over filesystem interaction with libs/vfs ([#1452](#1452)).
 * Add `filer.Filer` to read notebooks from WSFS without omitting their extension ([#1457](#1457)).
 * Fix listing notebooks in a subdirectory ([#1468](#1468)).

API Changes:
 * Changed `databricks account storage-credentials list` command to return .
 * Added `databricks consumer-listings batch-get` command.
 * Added `databricks consumer-providers batch-get` command.
 * Removed `databricks apps create-deployment` command.
 * Added `databricks apps deploy` command.

OpenAPI commit 37b925eba37dfb3d7e05b6ba2d458454ce62d3a0 (2024-06-03)

Dependency updates:
 * Bump github.com/hashicorp/go-version from 1.6.0 to 1.7.0 ([#1454](#1454)).
 * Bump github.com/hashicorp/hc-install from 0.6.4 to 0.7.0 ([#1453](#1453)).
@pietern pietern mentioned this pull request Jun 4, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jun 4, 2024
CLI:
* Update OpenAPI spec
([#1466](#1466)).

Bundles:
* Upgrade TF provider to 1.46.0
([#1460](#1460)).
* Add support for Lakehouse monitoring
([#1307](#1307)).
* Make dbt-sql and default-sql templates public
([#1463](#1463)).

Internal:
* Abstract over filesystem interaction with libs/vfs
([#1452](#1452)).
* Add `filer.Filer` to read notebooks from WSFS without omitting their
extension ([#1457](#1457)).
* Fix listing notebooks in a subdirectory
([#1468](#1468)).

API Changes:
* Changed `databricks account storage-credentials list` command to
return .
 * Added `databricks consumer-listings batch-get` command.
 * Added `databricks consumer-providers batch-get` command.
 * Removed `databricks apps create-deployment` command.
 * Added `databricks apps deploy` command.

OpenAPI commit 37b925eba37dfb3d7e05b6ba2d458454ce62d3a0 (2024-06-03)

Dependency updates:
* Bump github.com/hashicorp/go-version from 1.6.0 to 1.7.0
([#1454](#1454)).
* Bump github.com/hashicorp/hc-install from 0.6.4 to 0.7.0
([#1453](#1453)).
github-merge-queue bot pushed a commit that referenced this pull request Nov 14, 2024
## Changes

This field was special-cased in #1307 because it's not part of the JSON
payload in the SDK struct.

This approach, while pragmatic, meant it didn't show up in the JSON
schema. While debugging an issue with quality monitors in #1900, I
couldn't figure out why I was getting schema errors on this field, or
how it was passed through to the TF representation. This commit removes
the special case and makes it behave like everything else.

## Tests

* Unit tests pass.
* Confirmed that the updated schema failed validation before this
change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants