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

Summarize report as HTML: the gin is out of the bottle #87

Merged
merged 15 commits into from
Dec 12, 2024

Conversation

rohit-nayak-ps
Copy link
Contributor

@rohit-nayak-ps rohit-nayak-ps commented Dec 9, 2024

  • Background web server runs on demand (flag based) on the Gin framework
  • Added initial template for Summarize
  • Needed to refactor attributes of several classes to be public or use string map keys so that they are JSON marshallable

Todos:

  • Not all sections are tested yet (errors, transactions are pending)
  • Improve styles for a first cut
  • Cleanup code

Future PRs:

  • create single SummaryView class with required denormalized attributes and move Markdown and HTML to use the same one

Signed-off-by: Rohit Nayak <[email protected]>
…alled json. Change map key from ColumnInformation to a string representation so it can be marshalled. Fix tests. Code for launching summary is WIP

Signed-off-by: Rohit Nayak <[email protected]>
Signed-off-by: Rohit Nayak <[email protected]>
Signed-off-by: Rohit Nayak <[email protected]>
Signed-off-by: Rohit Nayak <[email protected]>
Signed-off-by: Rohit Nayak <[email protected]>
Signed-off-by: Rohit Nayak <[email protected]>
Signed-off-by: Rohit Nayak <[email protected]>
Signed-off-by: Rohit Nayak <[email protected]>
@rohit-nayak-ps rohit-nayak-ps changed the title Experimental: Report Summarize as HTML: the gin is out of the bottle Summarize report as HTML: the gin is out of the bottle Dec 10, 2024
@rohit-nayak-ps rohit-nayak-ps marked this pull request as ready for review December 11, 2024 09:52
Signed-off-by: Rohit Nayak <[email protected]>
Comment on lines +39 to +49
RunE: func(_ *cobra.Command, _ []string) error {
if port > 0 {
if webserverStarted {
return nil
}
webserverStarted = true
go startWebServer(port, ch)
<-ch
}
return nil
},
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I understand why the web server is called in the root command (and thus for all other commands). We could instead do something similar to what we did for the graphs where we only spin up the web server at the end of summarize when the *Summary object is constructed. That way the logic would be local to summarize and the web server will be created at a time where we already have all the information we wish to render.

Copy link
Member

Choose a reason for hiding this comment

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

However, I don't necessarily think we need to change this in this PR. We can change it in a subsequent PR if we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason to do it globally is that I was not sure if we wanted to extend the usage of the web server for other purposes, so kept it global. I added the Home/About pages to give a trivial example of what is possible.

I think as we progress we will be persisting state of previous runs and then this website can serve as a way to look at them. Also we should move the graphing to be part of this ...

@@ -40,7 +40,7 @@ type (
Value int `json:"value"`
Type string `json:"type"`
Curvature float64 `json:"curvature"`
Predicates []string `json:"predicates"`
Predicates []string `json:"Predicates"`
Copy link
Member

Choose a reason for hiding this comment

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

any reason for this change?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Predicates []string `json:"Predicates"`
Predicates []string `json:"predicates"`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, that is probably a Refactor/Rename issue. Will change

Comment on lines +165 to 180
func writeToTempFile(summaryJSON []byte) (*os.File, error) {
tmpFile, err := os.CreateTemp("/tmp/", "vt-summary-*.json")
if err != nil {
fmt.Println("Error creating temp file:", err)
return nil, err
}
return s, nil
defer tmpFile.Close()

_, err = tmpFile.WriteString(string(summaryJSON))
if err != nil {
fmt.Println("Error writing to temp file:", err)
return nil, err
}

return tmpFile, err
}
Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think we could avoid this step by creating and starting the web server directly from here instead of creating temporary files that will be used by the server created beforehand. Moving the logic here will give us more control over what and how we render.

@@ -102,24 +102,24 @@ Each object in the query-signatures array represents a generalized query and inc
* op: The operation type (e.g., "insert", "update", "delete").
* affected_table: The table affected by the query.
* updated_columns: (Only for update operations) An array of column names that are updated by the query.
* predicates: An array of conditions (also known as predicates) used in the query’s WHERE clause. Each predicate abstracts the condition to focus on the pattern rather than specific values. Not all predicates are included in the query signature; only those that could be used by the planner to select if the transaction is a single shard or a distributed transaction.
* Predicates: An array of conditions (also known as Predicates) used in the query’s WHERE clause. Each predicate abstracts the condition to focus on the pattern rather than specific values. Not all Predicates are included in the query signature; only those that could be used by the planner to select if the transaction is a single shard or a distributed transaction.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Predicates: An array of conditions (also known as Predicates) used in the query’s WHERE clause. Each predicate abstracts the condition to focus on the pattern rather than specific values. Not all Predicates are included in the query signature; only those that could be used by the planner to select if the transaction is a single shard or a distributed transaction.
* predicates: An array of conditions (also known as predicates) used in the query’s WHERE clause. Each predicate abstracts the condition to focus on the pattern rather than specific values. Not all predicates are included in the query signature; only those that could be used by the planner to select if the transaction is a single shard or a distributed transaction.


```json
"predicates": [
"Predicates": [
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Predicates": [
"predicates": [


### Example Explained

Consider the following predicates array:
Consider the following Predicates array:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Consider the following Predicates array:
Consider the following predicates array:

* table: The name of the table referenced in the condition.
* col: The column name used in the condition.
* op: A code representing the comparison operator used in the condition. For example:
- 0 might represent the "=" operator.
- Other numbers might represent different operators like <, >, LIKE, etc.
* val: A generalized placeholder value used in the condition. Instead of showing specific values, placeholders are used to indicate where values are compared. Identical placeholders across different predicates suggest that the same variable or parameter is used. -1 is a special value that indicates a unique value used only by this predicate.
* val: A generalized placeholder value used in the condition. Instead of showing specific values, placeholders are used to indicate where values are compared. Identical placeholders across different Predicates suggest that the same variable or parameter is used. -1 is a special value that indicates a unique value used only by this predicate.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* val: A generalized placeholder value used in the condition. Instead of showing specific values, placeholders are used to indicate where values are compared. Identical placeholders across different Predicates suggest that the same variable or parameter is used. -1 is a special value that indicates a unique value used only by this predicate.
* val: A generalized placeholder value used in the condition. Instead of showing specific values, placeholders are used to indicate where values are compared. Identical placeholders across different predicates suggest that the same variable or parameter is used. -1 is a special value that indicates a unique value used only by this predicate.


#### Inside Each Predicate

Each predicate object in the predicates array includes:
Each predicate object in the Predicates array includes:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Each predicate object in the Predicates array includes:
Each predicate object in the predicates array includes:

@@ -39,7 +39,7 @@ The output JSON file contains an array of transaction patterns, each summarizing
"updated_columns": [
"apa"
],
"predicates": [
"Predicates": [
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Predicates": [
"predicates": [

@@ -60,7 +60,7 @@ The output JSON file contains an array of transaction patterns, each summarizing
"updated_columns": [
"monkey"
],
"predicates": [
"Predicates": [
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Predicates": [
"predicates": [

@@ -138,7 +138,7 @@ Consider the following predicates array:
* The first predicate represents a condition on tblA.foo, using the operator code 0 (e.g., "="), with a generalized value 0.
* The second predicate represents a condition on tblA.id, also using the operator code 0, with a generalized value -1. That means that this value was only used by this predicate and not shared by any other queries in the transaction.

This numbering helps identify the relationships between different predicates in the transaction patterns and can be used to help guide choices in sharding strategies.
This numbering helps identify the relationships between different Predicates in the transaction patterns and can be used to help guide choices in sharding strategies.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This numbering helps identify the relationships between different Predicates in the transaction patterns and can be used to help guide choices in sharding strategies.
This numbering helps identify the relationships between different predicates in the transaction patterns and can be used to help guide choices in sharding strategies.

Copy link
Member

@frouioui frouioui left a comment

Choose a reason for hiding this comment

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

Only comment is /Predicate/predicate/ otherwise looks good to me! 🙇🏻

Signed-off-by: Rohit Nayak <[email protected]>
@rohit-nayak-ps rohit-nayak-ps merged commit 1390f1c into vitessio:main Dec 12, 2024
1 check passed
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.

2 participants