Support Lakehouse monitoring in bundles #1307

aravind-segu · 2024-03-24T18:52:46Z

Changes

Support Lakehouse Monitoring in Bundles

Testing

Unit Tests

libs/dyn/convert/normalize.go

aravind-segu · 2024-03-24T18:54:23Z

libs/dyn/convert/struct_info.go

 				continue
 			}
+			if name == "-" {


The full_name parameter is a url parameter. Therefore the json is -. Added a case that if the json is - uses the Key name to use get the json name

This looks pretty hacky. Can't we use the url tag of the field?

@pietern url tag is also - for path params but non-empty for query params. But I agree that it's pretty hacky and needs to be addressed on Go SDK side any maybe by introducing a new annotation like path: or so

Instead of this fix that looks at the URL we can come up with something else specifically for monitors.

This fix will break down in other cases so I rather not have a generic solution.

pietern · 2024-03-25T08:17:24Z

bundle/internal/tf/schema/resource_lakehouse_monitor.go

+    Snapshot *ResourceLakehouseMonitorSnapshot `json:"snapshot,omitempty"`
+    TimeSeries *ResourceLakehouseMonitorTimeSeries `json:"time_series,omitempty"`
+}
+


Do you have a similar PR to add LHM to the TF provider? I don't see it in the released versions yet.

Terraform LHM: databricks/terraform-provider-databricks#3238

bundle/config/resources.go

bundle/deploy/terraform/tfdyn/convert_lakehouse_monitor_test.go

pietern · 2024-03-25T08:19:15Z

bundle/schema/openapi.go

+	}
+	return lakehouseMonitorAllDocs, nil
+}
+


Why is this not generated as part of the main resources?

Ping on this.

libs/dyn/convert/normalize.go

pietern · 2024-03-25T08:21:49Z

libs/dyn/convert/struct_info.go

 				continue
 			}
+			if name == "-" {


This looks pretty hacky. Can't we use the url tag of the field?

pietern

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

bundle/config/mutator/process_target_mode_test.go

aravind-segu · 2024-04-15T16:52:39Z

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 · 2024-04-22T20:52:40Z

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?

andrewnester

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

bundle/deploy/terraform/convert_test.go

andrewnester · 2024-04-23T09:01:27Z

bundle/tests/lakehouse_monitor_test.go

+	assertExpectedLakehouseMonitor(t, p)
+}
+
+func TestLakehouseMonitorStaging(t *testing.T) {


All 3 tests are doing the same thing, we can just have 1 test the overrides.
Instead please add the test with overriding other options like output_schema_name and inference_log

andrewnester · 2024-04-23T09:17:35Z

libs/dyn/convert/struct_info.go

 				continue
 			}
+			if name == "-" {


@pietern url tag is also - for path params but non-empty for query params. But I agree that it's pretty hacky and needs to be addressed on Go SDK side any maybe by introducing a new annotation like path: or so

bundle/deploy/terraform/tfdyn/convert_lakehouse_monitor.go

codecov-commenter · 2024-05-06T03:11:45Z

Codecov Report

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

Project coverage is 53.82%. Comparing base (e22dd8a) to head (49ffedc).
Report is 118 commits behind head on main.

Files	Patch %	Lines
bundle/config/resources.go	0.00%	15 Missing and 3 partials ⚠️
bundle/schema/openapi.go	0.00%	16 Missing ⚠️
libs/dyn/convert/struct_info.go	18.18%	8 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.82%   +1.56%     
==========================================
  Files         317      347      +30     
  Lines       18004    20013    +2009     
==========================================
+ Hits         9408    10771    +1363     
- Misses       7903     8442     +539     
- Partials      693      800     +107

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

pietern · 2024-05-15T19:24:47Z

bundle/config/mutator/process_target_mode_test.go

@@ -145,6 +148,9 @@ func TestProcessTargetModeDevelopment(t *testing.T) {

 	// Registered model 1
 	assert.Equal(t, "dev_lennart_registeredmodel1", b.Config.Resources.RegisteredModels["registeredmodel1"].Name)
+
+	//Monitor 1


Suggested change

//Monitor 1

// Monitor 1

bundle/config/mutator/run_as_test.go

bundle/deploy/terraform/convert_test.go

pietern · 2024-05-15T19:27:20Z

bundle/schema/openapi.go

+	}
+	return lakehouseMonitorAllDocs, nil
+}
+


Ping on this.

pietern · 2024-05-15T19:28:39Z

bundle/tests/monitor/databricks.yml

+          table_name: "main.test.prod"
+          output_schema_name: "prod"
+          inference_log:
+            granularities: ["1 hour"]


Lists are appended to for overrides, implying we would see 2 granularities here.

Yes they are appended, for the user this would granularities for aggregating data into time windows. Should we add a comment in the documentation somewhere?

pietern · 2024-05-15T20:01:14Z

libs/dyn/convert/struct_info.go

 				continue
 			}
+			if name == "-" {


Instead of this fix that looks at the URL we can come up with something else specifically for monitors.

This fix will break down in other cases so I rather not have a generic solution.

aravind-segu requested a review from arpitjasa-db March 24, 2024 18:52

aravind-segu commented Mar 24, 2024

View reviewed changes

libs/dyn/convert/normalize.go Outdated Show resolved Hide resolved

aravind-segu commented Mar 24, 2024

View reviewed changes

pietern changed the title ~~[MLOPS-651] Support Lakehouse monitoring in Bundles~~ Support Lakehouse monitoring in bundles Mar 25, 2024

pietern reviewed Mar 25, 2024

View reviewed changes

andrewnester reviewed Mar 25, 2024

View reviewed changes

bundle/config/mutator/process_target_mode_test.go Outdated Show resolved Hide resolved

aravind-segu force-pushed the lakehouseMonitoring branch from 8bbbcdb to ea7b74b Compare April 15, 2024 16:13

andrewnester requested changes Apr 23, 2024

View reviewed changes

aravind-segu force-pushed the lakehouseMonitoring branch from a915754 to cfe77aa Compare May 6, 2024 03:08

aravind-segu requested a review from andrewnester May 6, 2024 03:16

arpitjasa-db requested a review from pietern May 6, 2024 20:33

pietern reviewed May 15, 2024

View reviewed changes

aravind-segu added 9 commits May 23, 2024 14:15

Add Lakehouse Monitor

e3645f2

clean up

4e1789b

remove substituions

a55ef31

Remove print statements

f642798

Add assertions and minor bug fix

7307260

Address pr comments

51e3801

Fix Tests

0f808ac

Remove prints and update to monitor

d7ae5e9

update to 1.45 and rename to quality monitors

c240c31

aravind-segu force-pushed the lakehouseMonitoring branch from 8d4a1a9 to c240c31 Compare May 25, 2024 00:19

Update test

49ffedc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Lakehouse monitoring in bundles #1307

Support Lakehouse monitoring in bundles #1307

aravind-segu commented Mar 24, 2024

aravind-segu Mar 24, 2024

pietern Mar 25, 2024

andrewnester Apr 23, 2024

pietern May 15, 2024

pietern Mar 25, 2024

aravind-segu Apr 15, 2024

pietern Mar 25, 2024

pietern May 15, 2024

pietern Mar 25, 2024

pietern left a comment

aravind-segu commented Apr 15, 2024

aravind-segu commented Apr 22, 2024

andrewnester left a comment •

edited

andrewnester Apr 23, 2024

andrewnester Apr 23, 2024

codecov-commenter commented May 6, 2024 •

edited

pietern May 15, 2024

pietern May 15, 2024

pietern May 15, 2024

aravind-segu May 25, 2024

pietern May 15, 2024

Support Lakehouse monitoring in bundles #1307

Are you sure you want to change the base?

Support Lakehouse monitoring in bundles #1307

Conversation

aravind-segu commented Mar 24, 2024

Changes

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pietern left a comment

Choose a reason for hiding this comment

aravind-segu commented Apr 15, 2024

aravind-segu commented Apr 22, 2024

andrewnester left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 6, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewnester left a comment •

edited

codecov-commenter commented May 6, 2024 •

edited