[Uptime] Fix Name/ID column label on overview #32603

andrewvc · 2019-03-07T01:44:24Z

This column was named ID even though it preferentially shows the monitor name. This patch replaces the table heading with the text Name / ID and adds a disambiguating title to the links. Additionally, it italicizes IDs to help distinguish them from names.

Goes toward resolving #27752.

Before

After

This column was named `ID` even though it preferentially shows the monitor name. This patch replaces the table heading with the text `Name / ID` and adds a disambiguating title to the links. Additionally, it italicizes IDs to help distinguish them from names. Goes toward resolving elastic#27752.

elasticmachine · 2019-03-07T01:44:26Z

Pinging @elastic/uptime

elasticmachine · 2019-03-07T02:37:15Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

andrewvc · 2019-03-07T15:02:22Z

Part of me thinks we should just have two columns, but that would use a ton of space.

justinkambic

This change is definitely nicer than what we're currently doing IMO.

We should get feedback from someone on @elastic/design as well.

I left a few comments asking for changes that are debatable.

justinkambic · 2019-03-07T18:48:08Z

x-pack/plugins/uptime/public/components/functional/monitor_list.tsx

+              return (
+                <Link
+                  to={`/monitor/${id}`}
+                  title={`Name: ${name ? "'" + name + "'" : 'N/A'}, ID: '${id}'`}


Suggested change

title={`Name: ${name ? "'" + name + "'" : 'N/A'}, ID: '${id}'`}

title={`Name: ${name ? `"${name}"` : 'N/A'}\nID: "${id}"`}

I think it's better to consistently use the template-style string definition.
Additionally, I think it's nicer and easier to read the title if we add a newline. WDYT?

justinkambic · 2019-03-07T18:50:17Z

x-pack/plugins/uptime/public/components/functional/monitor_list.tsx

@@ -79,16 +80,20 @@ export const MonitorList = ({ dangerColor, loading, monitors, primaryColor }: Mo
          },
          {
            field: 'ping.monitor.id',
-            name: i18n.translate('xpack.uptime.monitorList.idColumnLabel', {
-              defaultMessage: 'ID',
+            name: i18n.translate('xpack.uptime.monitorList.nameIdColumnLabel', {


Thank you for updating key. Can you please add a description as well?
The key for it is description, it's a sibling of defaultMessage and it takes a string.

cchaos · 2019-03-07T18:58:59Z

I don't think it's necessary to italicize the link if it's an ID. It's not obvious that that's what it is signifying. I think the actual text itself is pretty recognizable as names are more human-readable words and id's are a mash of letters, numbers and dashes.

Is there any chance that there can be both a name and ID? If so, I'd guess that you'd want to display both in which case, I'd stack them like:

Name of the thing
id of the thing (no link)

andrewvc · 2019-03-07T20:54:36Z

@cchaos all items have an ID, it is recommended that all items have both a Name and an ID.

I do like the stacking. It brings up an additional question: in the case where there's no name how would you handle the linking?

cchaos · 2019-03-07T21:32:19Z

Then you can just add the link to the ID instead. Unless you really want to re-enforce creating names, then you can maybe do:

[Unnamed monitor]
id of the thing

andrewvc · 2019-03-07T21:36:16Z

I prefer the re-enforcement of the naming. Also, I think it's weird that sometimes IDs are linked, and sometimes they are not, so I'll go with the bracketed solution you propose.

cchaos · 2019-03-07T21:38:24Z

cc @gchaps For guidance on what unnamed items should actually be called?

gchaps · 2019-03-07T22:04:45Z

I'd use Unnamed, but give the text some kind of style change. Because the user could actually have used the named Unnamed.

[Unnamed]
id of the thing

elasticmachine · 2019-03-12T00:07:23Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-12T11:24:40Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

andrewvc · 2019-03-12T20:18:29Z

Thanks for the help @cchaos and @gchaps . This is going to slip 7.0 because I'm realizing we need this change across both the monitor and error list. (CC @dov0211 )

That means refactoring some APIs and the way we query things. We'll need to switch to a two-phase query where we search the time slice indicated by the picker to generate charts etc, but then issue a second query to get the most recent state for things like:

the name/id column (since the name can change over time with a stable ID)
the status indicator, which should always be current I think. Would like some feedback on this point from @dov0211 @makwarth and @justinkambic

andrewvc · 2019-03-12T20:19:03Z

As a heads up, I've moved this PR to WIP as I work through these larger changes.

elasticmachine · 2019-03-12T20:42:32Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

dov0211 · 2019-03-14T10:31:06Z

I had looked at this PR
Looks like there is a problem with the error table

Also, why should we display the ID when a name is defined?

andrewvc · 2019-03-18T21:18:20Z

@dov0211 yes, this is definitely an in-progress PR.

The question of whether to display the ID is a good one. The ID is useful for users using yml config files, which is the only way to do it today. However, once we have centralized config it won't be useful.

Would love to hear some alternative ideas (and see mocks) that would work for both camps. One idea is that we could hide the IDs for centrally configured monitors.

andrewvc · 2019-03-20T00:35:38Z

After meeting with @dov0211 and @justinkambic we decided to only have the name field shown. Since IDs will be hidden with the upcoming management UI, they need not live in most tables. We'll keep them available, for now, on the monitor detail pages, but will potentially de-emphasize them in the future.

andrewvc · 2019-03-26T21:31:30Z

@justinkambic @dov0211 the monitor name is now shown on both the monitor list and the error list.

This also reworks how we calculate the name, pulling the latest name value, so even if a user changes the name, the correct value is shown.

This also refactors some internals to be more clean, removing the notion of id{key, url} and replacing it with a simple string since only 6.7 needed that.

Those two refactors account for most of the code changes here.

elasticmachine · 2019-03-26T21:46:05Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-26T23:38:58Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-27T00:26:19Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

justinkambic

There're a few broken unit tests that need some snapshot refreshes.

I'm considering making a PR to update our README with a Contributing checklist so that contributors (and me) can more easily get their PRs production-ready. WDYT?

justinkambic · 2019-03-12T20:38:00Z

x-pack/plugins/uptime/public/components/functional/monitor_name_id.tsx

+export const MonitorNameID = (id: string, monitor: LatestMonitor) => {
+  const name = get(monitor, 'ping.monitor.name');
+  return (
+    <div>


I'm assuming you have a <div> here because React requires there to be a single root-level element. Unless there's a stylistic reason for <div>, it is probably better to use Fragment here.

justinkambic · 2019-03-12T20:40:06Z

x-pack/plugins/uptime/server/lib/adapters/monitors/elasticsearch_monitors_adapter.ts

+  /**
+   * Fetches the latest version of each monitor, regardless of time picker settings
+   * This is important because we usually want to display the latest name and other info
+   * for a given monitor


👍 thanks for the comment + params description

justinkambic · 2019-03-26T21:47:49Z

x-pack/plugins/uptime/public/components/functional/monitor_name.tsx

+  name?: string | null;
+}
+
+export const MonitorName: React.SFC<MonitorNameProps> = ({ id, name }) => {


++ on this being its own component

justinkambic · 2019-03-26T22:11:21Z

x-pack/plugins/uptime/public/components/functional/monitor_name.tsx

+  return (
+    <div>
+      <EuiLink href={`#/monitor/${id}`} className="ut-monitor-name">
+        {name ? name : <em>[Unnamed]</em>}


This needs to be translated. Something like this is what we'll need:

{name ? ( name ) : ( <em> {i18n.translate('few', { defaultMessage: '[Unnamed]', description: 'This is the default text we show when an uptime monitor has no name defined', })} </em> )}

justinkambic · 2019-03-27T17:14:01Z

x-pack/plugins/uptime/server/lib/helper/get_filtered_query.ts

@@ -7,8 +7,8 @@
 import { get, set } from 'lodash';

 export const getFilteredQuery = (
-  dateRangeStart: string,
-  dateRangeEnd: string,
+  dateRangeStart: string | null,


Can you add some unit tests to cover the contingencies introduced in this PR?

It looks like we'd need tests to cover

Only dateRangeStart is null

Only dateRangeEnd is null

!dateRangeStart && !dateRangeEnd
where dateRangeStart and/or dateRangeEnd are null?

Snapshot tests are sufficient, just making sure that the returned value for each matches what you're expecting.

These functions are prime contenders for defect injection so having them well-tested averts problems and makes future additions easier. We're adding even more complexity for the time being, so this would be a big help.

justinkambic · 2019-03-27T17:18:53Z

x-pack/plugins/uptime/server/lib/adapters/monitors/elasticsearch_monitors_adapter.ts


+    const monitorIds = flatten(


Out of scope for this PR but what would you think of extracting this ancillary business logic to additional helper functions? There's a lot of untested code like this in these adapter files, and they're getting taller over time.

elasticmachine · 2019-03-28T03:56:45Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

dov0211 · 2019-04-10T12:42:47Z

Found the following error in the UI in the error table

with the following exception

Error: Cannot return null for non-nullable field LatestMonitor.name.
at completeValue (/Users/dov/kibana/node_modules/graphql/execution/execute.js:631:13)
at completeValueWithLocatedError (/Users/dov/kibana/node_modules/graphql/execution/execute.js:580:21)
at completeValueCatchingError (/Users/dov/kibana/node_modules/graphql/execution/execute.js:550:12)
at resolveField (/Users/dov/kibana/node_modules/graphql/execution/execute.js:497:10)
at /Users/dov/kibana/node_modules/graphql/execution/execute.js:364:18
at Array.reduce ()
at executeFields (/Users/dov/kibana/node_modules/graphql/execution/execute.js:361:42)
at collectAndExecuteSubfields (/Users/dov/kibana/node_modules/graphql/execution/execute.js:772:10)
at completeObjectValue (/Users/dov/kibana/node_modules/graphql/execution/execute.js:762:10)
at completeValue (/Users/dov/kibana/node_modules/graphql/execution/execute.js:660:12)
at completeValueWithLocatedError (/Users/dov/kibana/node_modules/graphql/execution/execute.js:580:21)
at completeValueCatchingError (/Users/dov/kibana/node_modules/graphql/execution/execute.js:556:21)
at resolveField (/Users/dov/kibana/node_modules/graphql/execution/execute.js:497:10)
at /Users/dov/kibana/node_modules/graphql/execution/execute.js:364:18
at Array.reduce ()
at executeFields (/Users/dov/kibana/node_modules/graphql/execution/execute.js:361:42)
at collectAndExecuteSubfields (/Users/dov/kibana/node_modules/graphql/execution/execute.js:772:10)
at completeObjectValue (/Users/dov/kibana/node_modules/graphql/execution/execute.js:762:10)
at completeValue (/Users/dov/kibana/node_modules/graphql/execution/execute.js:660:12)
at completeValue (/Users/dov/kibana/node_modules/graphql/execution/execute.js:629:21)
at completeValueWithLocatedError (/Users/dov/kibana/node_modules/graphql/execution/execute.js:580:21)
at completeValueCatchingError (/Users/dov/kibana/node_modules/graphql/execution/execute.js:550:12)

alvarolobato · 2019-06-19T12:35:51Z

This work will be addressed during #38786

elasticmachine · 2019-06-19T13:14:23Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

andrewvc added bug Fixes for quality problems that affect the customer experience review Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Mar 7, 2019

andrewvc self-assigned this Mar 7, 2019

andrewvc requested review from justinkambic and dov0211 March 7, 2019 01:44

andrewvc mentioned this pull request Mar 7, 2019

[Uptime][META] 7.0 Punchlist #27752

Closed

7 tasks

justinkambic reviewed Mar 7, 2019

View reviewed changes

Add unnamed label

bf93f42

andrewvc added 2 commits March 12, 2019 05:50

Add explicit CSS classes for name/ID

ed73826

Merge remote-tracking branch 'origin/master' into fix-name-id-label

5b3de86

andrewvc added 2 commits March 12, 2019 07:44

Checkpoint

6174c23

Checkpoint for refactor of queries

57f6665

andrewvc added WIP Work in progress and removed review labels Mar 12, 2019

andrewvc added 3 commits March 26, 2019 11:52

Only show monitor names

8f1d710

moar

16c90ac

Merge remote-tracking branch 'origin/master' into fix-name-id-label

3682552

andrewvc added review and removed WIP Work in progress labels Mar 26, 2019

Make CI happy

1dcfe71

fix types

f7faf11

justinkambic requested changes Mar 27, 2019

View reviewed changes

justinkambic added v8.0.0 v7.2.0 labels Mar 27, 2019

Merge remote-tracking branch 'origin/master' into fix-name-id-label

5d1e83f

alvarolobato added :uptime and removed Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.2.0 labels Jun 19, 2019

alvarolobato closed this Jun 19, 2019

alvarolobato removed the [zube]: Ready label Jun 19, 2019

	title={`Name: ${name ? "'" + name + "'" : 'N/A'}, ID: '${id}'`}
	title={`Name: ${name ? `"${name}"` : 'N/A'}\nID: "${id}"`}

[Uptime] Fix Name/ID column label on overview #32603

[Uptime] Fix Name/ID column label on overview #32603

Conversation

andrewvc commented Mar 7, 2019

elasticmachine commented Mar 7, 2019

elasticmachine commented Mar 7, 2019

💚 Build Succeeded

andrewvc commented Mar 7, 2019

justinkambic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchaos commented Mar 7, 2019

andrewvc commented Mar 7, 2019

cchaos commented Mar 7, 2019

andrewvc commented Mar 7, 2019 • edited Loading

cchaos commented Mar 7, 2019

gchaps commented Mar 7, 2019

elasticmachine commented Mar 12, 2019

💔 Build Failed

elasticmachine commented Mar 12, 2019

💔 Build Failed

andrewvc commented Mar 12, 2019

andrewvc commented Mar 12, 2019

elasticmachine commented Mar 12, 2019

💔 Build Failed

dov0211 commented Mar 14, 2019 • edited Loading

andrewvc commented Mar 18, 2019

andrewvc commented Mar 20, 2019

andrewvc commented Mar 26, 2019

elasticmachine commented Mar 26, 2019

💔 Build Failed

elasticmachine commented Mar 26, 2019

💔 Build Failed

elasticmachine commented Mar 27, 2019

💔 Build Failed

justinkambic left a 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

elasticmachine commented Mar 28, 2019

💔 Build Failed

dov0211 commented Apr 10, 2019

alvarolobato commented Jun 19, 2019

elasticmachine commented Jun 19, 2019

💔 Build Failed

andrewvc commented Mar 7, 2019 •

edited

Loading

dov0211 commented Mar 14, 2019 •

edited

Loading