Implement Migration Complexity Explanation providing the summary and rationale for reported complexity #2166

sanyamsinghal · 2025-01-09T08:21:53Z

Describe the changes in this pull request

Different explanation in case of html and json. Html also includes the summary about categories and issues count.

https://yugabyte.atlassian.net/browse/DB-14717

Describe if there are any user-facing changes

Analyze Schema report has stopped reporting migration complexity
Assessment Report will have one more field - MigrationComplexityExplanation

Sample HTML Report

Non zero issues

Zero Issues

Sample JSON Report

How was this pull request tested?

Manually.
This is a new field in the assessment report which we plan to add to test later with other UI changes.
Also have to modified the upgrade unit test for the new field.

Does your PR have changes that can cause upgrade issues?

Component	Breaking changes?
MetaDB	No
Name registry json	No
Data File Descriptor Json	No
Export Snapshot Status Json	No
Import Data State	No
Export Status Json	No
Data .sql files of tables	No
Export and import data queue	No
Schema Dump	No
AssessmentDB	No
Sizing DB	No
Migration Assessment Report Json	No
Callhome Json	No
YugabyteD Tables	No
TargetDB Metadata Tables	No

…nd rationalte for complexity reported

…fied

makalaaneesh · 2025-01-09T09:02:49Z

yb-voyager/cmd/migration_complexity.go

+
+// ======================================= Migration Complexity Explanation ==========================================
+
+const explainTemplateHTML = `


Would like to avoid having HTML in multiple places. It should all ideally be in the template file. Morevoer, field AssessmentReport.MigrationComplexityExplanation having HTML would be weird.
But I get why you did this. (because json and HTML have different requirements).

Here's one way to do this:

in AssessmentReport, instead of storing just the final string in MigrationComplexityExplanation, store the actual ExplanationData struct.

in HTML template, you have all the necessary information to build the table there.

for json, just exclude the summaries from it. (in fact there's no harm in even keeping it tbh).

Agree to this, we should keep all the HTML code in that template

for json, just exclude the summaries from it. (in fact there's no harm in even keeping it tbh).

yes we can keep it in JSON as well, in case of tests and all you are anyways ignoring it

added a TODO comment

yb-voyager/cmd/migration_complexity.go

yb-voyager/src/utils/utils.go

yb-voyager/cmd/migration_complexity.go

priyanshi-yb · 2025-01-09T09:00:00Z

yb-voyager/cmd/assessMigrationCommand.go

+	var err error
+	assessmentReport.MigrationComplexityExplanation, err = buildMigrationComplexityExplanation(source.DBType, assessmentReport, "")
+	if err != nil {
+		utils.PrintAndLog("ERROR: unable to build migration complexity explanation for json report: %v", err)


why utils.PrintAndLog, use return fmt.Errorf(..) ?

I was thinking of showing something on console also for users to know that explanation is not there for xyz reason

The top level functions which is calling the generateAssessmentReportJson must be printing the error on console for users so it will come eventually right?

will this not look good? any suggestions?

The top level functions which is calling the generateAssessmentReportJson must be printing the error on console for users so it will come eventually right?

No, what i did was to not error out the cmd if building explanation fails...

are you suggesting to error out in this scenario?

Oh, I see. Ideally, we should return the error,and mostly the errors occur while buildMigrationComplexityExplanation is generating HTML string, etc.
For the case, we should fail the command for this new addition, we can use env var approach and if user faces any blocker we can always ask them to disable it.

hmm makes sense, this is the standard code - creating template and executing it.
Should not be error here ideally.

yb-voyager/cmd/assessMigrationCommand.go

yb-voyager/cmd/common.go

yb-voyager/cmd/migration_complexity.go

makalaaneesh · 2025-01-09T11:07:08Z

yb-voyager/cmd/migration_complexity.go

+
+// TODO: discuss if the html should be in main report or here
+const explainTemplateHTML = `
+<p>Below is a detailed breakdown of issues by category, showing the count for each impact level.</p>


How about Below is a breakdown of the issues detected in different categories for each impact level ?

yeah better.

makalaaneesh

LGTM

priyanshi-yb

LGTM

priyanshi-yb · 2025-01-09T11:02:17Z

yb-voyager/cmd/migration_complexity.go

+
+	var result []MigrationComplexityCategorySummary
+	for _, summary := range summaryMap {
+		summary.Category = utils.SnakeCaseToTitleCase(summary.Category)


Instead of using this function we can get the Feature heading for that Enum to be consistent with the heading in report.
but given that we are anyways changing the UI, we can revisit it later so okay for now.

yeah lets revisit this. The category here is being fetched from the analyze schema issue.

I see the same issues of formatting in schema reports. We need to fix the root.

sanyamsinghal added 6 commits January 9, 2025 06:00

Implement Migration Complexity Explainability providing the summary a…

bf65641

…nd rationalte for complexity reported

changes as per feedback

028c168

minor UI changes

730da79

changing level definitions

cbbf84f

Ignoring MigrationComplexityExplanation field in the tests to be veri…

9869d5c

…fied

Fixed typo and modified upgrade test for assessment report

de90d2c

makalaaneesh reviewed Jan 9, 2025

View reviewed changes

priyanshi-yb reviewed Jan 9, 2025

View reviewed changes

hbhanawat reviewed Jan 9, 2025

View reviewed changes

yb-voyager/cmd/migration_complexity.go Outdated Show resolved Hide resolved

review comments

988fae3

sanyamsinghal requested review from makalaaneesh, hbhanawat and priyanshi-yb January 9, 2025 10:29

sanyamsinghal changed the title ~~Implement Migration Complexity Explainability providing the summary and rationale for reported complexity~~ Implement Migration Complexity Explanation providing the summary and rationale for reported complexity Jan 9, 2025

sanyamsinghal added 3 commits January 9, 2025 10:46

some UI changes as per feedback

4ac3167

review comments

c36b10a

added a TODO comment

df9ea11

sanyamsinghal self-assigned this Jan 9, 2025

makalaaneesh reviewed Jan 9, 2025

View reviewed changes

handled html UI for zero issues cases

25b150b

makalaaneesh approved these changes Jan 9, 2025

View reviewed changes

priyanshi-yb approved these changes Jan 9, 2025

View reviewed changes

sanyamsinghal merged commit 7802ecb into main Jan 9, 2025
67 checks passed

sanyamsinghal deleted the sanyam/mig-explainability branch January 9, 2025 11:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Migration Complexity Explanation providing the summary and rationale for reported complexity #2166

Implement Migration Complexity Explanation providing the summary and rationale for reported complexity #2166

sanyamsinghal commented Jan 9, 2025 •

edited

Loading

makalaaneesh Jan 9, 2025

priyanshi-yb Jan 9, 2025 •

edited

Loading

sanyamsinghal Jan 9, 2025

priyanshi-yb Jan 9, 2025

sanyamsinghal Jan 9, 2025

priyanshi-yb Jan 9, 2025 •

edited

Loading

sanyamsinghal Jan 9, 2025

sanyamsinghal Jan 9, 2025

sanyamsinghal Jan 9, 2025

priyanshi-yb Jan 9, 2025 •

edited

Loading

sanyamsinghal Jan 9, 2025

sanyamsinghal Jan 9, 2025

makalaaneesh Jan 9, 2025

sanyamsinghal Jan 9, 2025

makalaaneesh left a comment

priyanshi-yb left a comment

priyanshi-yb Jan 9, 2025

sanyamsinghal Jan 9, 2025


		// ======================================= Migration Complexity Explanation ==========================================

		const explainTemplateHTML = `

Implement Migration Complexity Explanation providing the summary and rationale for reported complexity #2166

Implement Migration Complexity Explanation providing the summary and rationale for reported complexity #2166

Conversation

sanyamsinghal commented Jan 9, 2025 • edited Loading

Describe the changes in this pull request

Describe if there are any user-facing changes

How was this pull request tested?

Does your PR have changes that can cause upgrade issues?

Choose a reason for hiding this comment

priyanshi-yb Jan 9, 2025 • edited Loading

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

priyanshi-yb Jan 9, 2025 • edited Loading

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

priyanshi-yb Jan 9, 2025 • edited Loading

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

makalaaneesh left a comment

Choose a reason for hiding this comment

priyanshi-yb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanyamsinghal commented Jan 9, 2025 •

edited

Loading

priyanshi-yb Jan 9, 2025 •

edited

Loading

priyanshi-yb Jan 9, 2025 •

edited

Loading

priyanshi-yb Jan 9, 2025 •

edited

Loading