Add support for generic multicolumn tables #2109

fbarl · 2016-12-23T16:01:43Z

Resolves #1980. Prior to this story, we could only render Node Tables and Labels (renamed to Property Lists in this PR) in the node details panel. This PR introduces Generic Table as a separate UI component that enables us to render generic data in a multi-column format without associating rows to nodes.

Modeling

As @davkal reasoned, it makes sense to have Property Lists and Generic Tables as distinct components in the UI, so I made them fully independent there, but for backward compatibility reasons, I kept them under the old Table model on the backend, which I finally decided to extend like this:

// Column is the type for multi-column tables in the UI.
type Column struct {
	ID       string `json:"id"`
	Label    string `json:"label"`
	DataType string `json:"dataType"`
}

// Row is the type that holds the table data for the UI. Entries map from column ID to cell value.
type Row struct {
	ID      string            `json:"id"`
	Entries map[string]string `json:"entries"`
}

// Table is the type for a table in the UI.
type Table struct {
	ID              string   `json:"id"`
	Label           string   `json:"label"`
	Type            string   `json:"type"`
	Columns         []Column `json:"columns"`
	Rows            []Row    `json:"rows"`
	TruncationCount int      `json:"truncationCount,omitempty"`
}

// TableTemplate describes how to render a table for the UI.
type TableTemplate struct {
	ID        string            `json:"id"`
	Label     string            `json:"label"`
	Prefix    string            `json:"prefix"`
	Type      string            `json:"type"`
	Columns   []Column          `json:"columns"`
	FixedRows map[string]string `json:"fixedRows"`
}

where Columns is used only with the multicolumn-table template type and FixedRows in only supported for property-list. The main reason I decided not to support fixed rows for generic (multicolumn) tables is that it wasn't clear what the exact use-case might be and how they would be rendered in the UI.

Storing

To be consistent with how property lists are being stored on the node, I decided to use the node.Latest map with table prefixes to store generic tables as well. The only difference is that since a multi-column table is two-dimensional, the table value is determined not only by label, but by (row, column) pair. Storing whole rows in the Latest map was not an option for two reasons:

Latest is a string to string map so we can only use it to store strings.
I also thought it would be better to use latest info on the cell level instead of row.

To be able to differ between the rows, each of them has to be given an ID (instead of what used to be a label). For backward compatibility with the old reports, property lists label is indeed used as a row ID when sending a table to the UI.

Rendering in the UI

Generic table is sortable and supports different column types (sent through Column.DataType field on the backend). Column headers are the same as for Node Tables (they are now in a new component called NodeDetailsTableHeaders), and column widths can also be hard-coded on the frontend.

To see this, run fixprobe on this report

Other Observations

Right now we iterate once through the whole node.Latest map for every table (or a property list) we are extracting, instead of just running through the entries with a given prefix and extracting the fixed row information separately. I haven't looked into the implementation details of our Map structure, but maybe it wouldn't be that difficult to add an efficient ForEachFromTo kind of method there to run only on selected intervals. That would drop our time complexity of extraction roughly from O(T * N) to O(N + T * log N) (or O(N * log N), if a lot of entries correspond to fixed rows), where T is the number of tables and N is the total size of node.Latest. Not sure if it's a bottleneck or if it's worth digging into this, but I just wanted to point it out.
There might other components in node-details dir in the UI, like NodeDetailsInfo, which are both semantically and implementation-wise similar to NodeDetailsPropertyList, so consider refactoring them in some future PR.

davkal

Very minor nits in the code.
Nicely documented PR.
Front-end side LGTM.

client/app/scripts/components/node-details.js

@@ -208,15 +213,13 @@ class NodeDetails extends React.Component {
              return (
                <div className="node-details-content-section" key={table.id}>
                  <div className="node-details-content-section-header">
-                    {table.label}
+                    {table.label.length > 0 && table.label}


client/app/scripts/components/node-details.js

 import NodeDetailsRelatives from './node-details/node-details-relatives';
 import NodeDetailsTable from './node-details/node-details-table';
 import Warning from './warning';

+
+const logError = debug('scope:error');


client/app/scripts/components/node-details/node-details-generic-table.js

+
+import { NODE_DETAILS_DATA_ROWS_DEFAULT_LIMIT } from '../../constants/limits';
+import {
+  isNumber, getTableColumnsStyles, genericTableEntryKey


client/app/scripts/components/node-details/node-details-generic-table.js

@@ -0,0 +1,108 @@
+import React from 'react';
+import { Map as makeMap } from 'immutable';
+import { sortBy } from 'lodash';


client/app/scripts/components/node-details/node-details-generic-table.js

+    super(props, context);
+    this.state = {
+      limit: NODE_DETAILS_DATA_ROWS_DEFAULT_LIMIT,
+      sortedBy: props.columns[0].id,


report/table.go

 // Table is the type for a table in the UI.
 type Table struct {
-	ID              string        `json:"id"`


report/table.go

+	switch template.Type {
+	case MulticolumnTableType:
+		rows = node.ExtractMulticolumnTable(template)
+	default: // By default assume it's a property list (for backward compatibility).


2opremio · 2017-01-10T17:24:47Z

@fbarl Really clean design and code.

Did you test with an old probe, to verify there are no backwards compatibility issues?

2opremio · 2017-01-10T17:27:25Z

To be consistent with how property lists are being stored on the node, I decided to use the node.Latest map with table prefixes to store generic tables as well

I've always thought that using the Latest map to store the metadata of tables was a bit of a hack (particularly for data only used in tables) since it didn't accommodate multidimensional data well and the latest-time conflict resolution is not needed in most cases. However, now that we are married with it (for backwards compatibility) I guess we can't cleanly store the metadata anywhere else.

report/table.go

+	sort.Strings(ids)
+
+	// Return the rows in the sorted order.
+	rows = []Row{}


report/table.go

+	})
+
+	// Gather a sorted list of rows' IDs.
+	ids := make([]string, 0, len(rowsByID))


report/table.go

+	}
+	sort.Strings(ids)
+
+	// Return the rows in the sorted order.


report/table.go

 		}
 	})
+
+	// Gather a sorted list of labels.


rade · 2017-01-10T23:03:37Z

Does this PR happen to address #1735 too?

fbarl · 2017-01-13T15:06:56Z

@rade Not really. All the column widths are still hardcoded on the frontend, I just moved that code to a different file :)

fbarl · 2017-01-13T15:21:12Z

@fons,

Did you test with an old probe, to verify there are no backwards compatibility issues?

I tested it on a manually modified report that I linked to above, in my PR description. Out of the three nodes visible in Weave Net, one of them has everything stored in the new format, one of them in the old, and one of them both (even though that case will never happen I think). I didn't know how to test this data dynamically (without loading it from static reports), so maybe we should do that additional check somehow before pushing. What do you think?

However, now that we are married (for backwards compatibility) I guess we can't cleanly store the metadata anywhere else.

While I was working on this story, I was gradually finding out how big of a constraint backwards compatibility is. Do you have any ideas how we could come around this in the future?

2opremio · 2017-01-16T10:43:04Z

so maybe we should do that additional check somehow before pushing. What do you think?

Your test sounds reasonable, but I would also run a standalone probe with an older version (e.g. Scope 1.1.0) to make sure everything renders as it should.

Do you have any ideas how we could come around this in the future?

Not really (without content negotiation or versioned APIs)

2opremio · 2017-01-16T10:43:26Z

Code looks good, waiting on further backwards compatibility testing to approve.

2opremio · 2017-01-16T10:44:05Z

@fbarl Would you mind (at least partially) squashing the commits?

Replaced MetadataRow with generic Row in Table model Sending through multicolumn tables from the backend

Rendering generic table columns Made Type a required attribute for TableTemplate Made generic table sortable on the UI

Extracted table headers common code on the frontend Fixed the search matching and extracted further common code in the UI

Backward-compatibility fix

fbarl · 2017-01-16T16:15:57Z

Code looks good, waiting on further backwards compatibility testing to approve.

I tested it with a standalone Scope 1.1.0 as you suggested @fons and good that I did because a small change on the backend still needed to be made for backwards compatibility with old probes to work properly. I also tested it with the new probe and with mixed reports after that, but I found no problems there. So as far as I could tell, everything should be working fine after merging this PR!

2opremio · 2017-01-16T16:19:21Z

Great job @fbarl . LGTM!

fbarl force-pushed the node-details-multicolumn-table branch 3 times, most recently from b7b4fbd to 9857c3a Compare January 4, 2017 14:54

fbarl self-assigned this Jan 6, 2017

fbarl force-pushed the node-details-multicolumn-table branch from 47bcdda to 6c93938 Compare January 6, 2017 11:28

fbarl changed the title ~~[WIP] Another attempt at generic multicolumn table support~~ Add support for generic multicolumn tables Jan 6, 2017

fbarl requested review from davkal and 2opremio January 6, 2017 11:30

davkal approved these changes Jan 6, 2017

View reviewed changes