Add status check to "add data" tutorials #17732

nreese · 2018-04-16T22:40:27Z

Updates tutorial schema by adding statusCheck configuration for instruction sets. Updates UI components to display status check step when statusCheck is present.

elasticmachine · 2018-04-16T23:27:44Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-04-27T19:43:23Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-04-30T12:41:05Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-04-30T15:07:58Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

stacey-gammon · 2018-05-01T12:02:42Z

src/core_plugins/kibana/public/home/components/tutorial/instruction_set.js

+            grow={false}
+          >
+            <EuiButton
+              onClick={() => {this.props.onStatusCheck();}}


nit: i think the spacing should be onClick={() => { this.props.onStatusCheck(); }}

or actually I think you can just do:

onClick={this.props.onStatusCheck}

stacey-gammon · 2018-05-01T12:13:35Z

src/core_plugins/kibana/public/home/components/tutorial/tutorial.js

+      const searchBody = JSON.stringify({ query: esHitsCheckConfig.query, size: 1 });
+      const body = `${searchHeader}\n${searchBody}\n`;
+
+      const response = await fetch(this.props.addBasePath('/elasticsearch/_msearch'), {


I think _msearch is for when there are multiple queries and here there is only ever one, so you should be able to get away with a single _search, if you adjust your query a bit. Not sure if it really makes any difference though, one way or the other. 🤷‍♀️ So take this feedback or leave it.

Using _search requires putting the index name in the URL and I could not get it work that way. Not sure if its because how the call gets proxied by the Kibana server.

elasticmachine · 2018-05-01T15:58:17Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

chrisdavies

LGTM w/ possible tweaks. Mostly unimportant stylistic things.

chrisdavies · 2018-05-01T16:12:57Z

src/core_plugins/kibana/public/home/components/tutorial/__snapshots__/tutorial.test.js.snap

@@ -54,8 +55,10 @@ exports[`isCloudEnabled is false should not render instruction toggle when ON_PR
            },
          ]
        }
+        isCheckingStatus={false}


I wonder if these should be an enum instead of a boolean, e.g.

<InstructionSet status={status} ... />

Where status is one of failed | checking | succeeded or whatever? It would allow for adding more scenarios later without continuing to layer in boolean properties.

chrisdavies · 2018-05-01T16:19:11Z

src/core_plugins/kibana/public/home/components/tutorial/instruction_set.js

+
+  renderStatusCheck() {
+    let statusMsg;
+    if (this.props.statusCheckState === 'complete') {


I'd consider moving this into its own method, e.g. something like:

statusMsg() { if (this.props.statusCheckState === 'complete') { return this.renderStatusCheckMsg(this.props.statusCheckConfig.success || 'Success', 'success'); } if (this.props.hasStatusCheckFailed) { return this.renderStatusCheckMsg(this.props.statusCheckConfig.error || 'No data found', 'warning'); } }

One other thought, based on the original index, is No data found a reasonable default or should there be some helpful explanation about it not being set up, possible mitigations, etc? Dunnoes.

chrisdavies · 2018-05-01T16:20:26Z

src/core_plugins/kibana/public/home/components/tutorial/instruction_set.js

+              onClick={this.props.onStatusCheck}
+              isLoading={this.props.isCheckingStatus}
+            >
+              {this.props.statusCheckConfig.btnLabel ? this.props.statusCheckConfig.btnLabel : 'Check status'}


I like to simplify these kinds of statements down to this:

this.props.statusCheckConfig.btnLabel || 'Check status'

chrisdavies · 2018-05-01T16:21:09Z

src/core_plugins/kibana/public/home/components/tutorial/instruction_set.js

+      </Fragment>
+    );
+    return {
+      title: this.props.statusCheckConfig.title ? this.props.statusCheckConfig.title : 'Status Check',


Same here:

title: this.props.statusCheckConfig.title || 'Status Check'

Not important, just being my typical nitpicky self. :)

chrisdavies · 2018-05-01T16:23:17Z

src/core_plugins/kibana/public/home/components/tutorial/tutorial.js

@@ -22,6 +22,7 @@ export class Tutorial extends React.Component {
    this.state = {
      notFound: false,
      paramValues: {},
+      statusCheck: [],  // array holding instruction set status check state


The comment here doesn't really add clarity (to me). I'd remove it, but alternatively, you could make it say something that the field name doesn't already indicate.

chrisdavies · 2018-05-01T17:01:03Z

src/core_plugins/kibana/public/home/components/tutorial/tutorial.js

@@ -104,6 +117,44 @@ export class Tutorial extends React.Component {
    });
  }

+  checkInstructionSetStatus = async (instructionSetIndex) => {
+    let isComplete = false;


Seems like a good candidate for a bit of a refactor. Additionally, I think it's considered preferable not to mutate component state within setState, so something like this, maybe?

checkInstructionSetStatus = async (instructionSetIndex) => { const instructions = this.getInstructions(); const esHitsCheckConfig = _.get(instructions, `instructionSets[${instructionSetIndex}].statusCheck.esHitsCheck`); const { hasFailed, isComplete } = await this.fetchStatus(esHitsCheckConfig); this.setState(({ statusCheck }) => ({ statusCheck: { ...statusCheck, [instructionSetIndex]: { ...statusCheck[instructionSetIndex], isComplete, hasFailed, isFetchingStatus: false, }, }, })); } async fetchStatus(esHitsCheckConfig) { if (!esHitsCheckConfig) { return { hasFailed: false, isComplete: false }; } const searchHeader = JSON.stringify({ index: esHitsCheckConfig.index }); const searchBody = JSON.stringify({ query: esHitsCheckConfig.query, size: 1 }); return await fetch(this.props.addBasePath('/elasticsearch/_msearch'), { method: 'post', body: `${searchHeader}\n${searchBody}\n`, headers: { accept: 'application/json', 'content-type': 'application/x-ndjson', 'kbn-xsrf': 'kibana', }, credentials: 'same-origin' }) .then(res => res.json()) .then(results => ({ hasFailed: false, isComplete: _.get(results, 'responses.[0].hits.hits.length', 0) > 0 })) .catch(() => ({ hasFailed: true, isComplete: false })); }

chrisdavies · 2018-05-01T17:04:32Z

src/core_plugins/kibana/public/home/components/tutorial/tutorial.js

+        statusCheckState = this.state.statusCheck[index].isComplete ? 'complete' : 'incomplete';
+        isCheckingStatus = this.state.statusCheck[index].isFetchingStatus;
+        hasFailed = this.state.statusCheck[index].hasFailed;
+      }


Could be condensed into this, I think:

const { statusCheckState, isCheckingStatus, hasFailed } = _.get(this, `state.statusCheck[${index}]`, {});

I believe it is more clear keeping it the way it is

chrisdavies · 2018-05-01T17:05:08Z

src/core_plugins/kibana/public/home/components/tutorial/tutorial.js

      return (
        <InstructionSet
          title={instructionSet.title}
          instructionVariants={instructionSet.instructionVariants}
+          statusCheckConfig={instructionSet.statusCheck}
+          statusCheckState={statusCheckState}
+          isCheckingStatus={isCheckingStatus}


If you do condense (per previous comment) these would need to be coerced, as they could be undefined:

isCheckingStatus={!!isCheckingStatus}

chrisdavies · 2018-05-01T17:15:15Z

src/core_plugins/kibana/public/home/components/tutorial/tutorial.js

+          isCheckingStatus={isCheckingStatus}
+          hasStatusCheckFailed={hasFailed}
+          onStatusCheck={() => {
+            this.setState((prevState) => {


checkInstructionSetStatus is already setting state within, so maybe, this should be simplified:

onStatusCheck={() => this.checkInstructionSetStatus(index)}

And checkInstructionSetStatus can be updated to do this before it does other things:

checkInstructionSetStatus = async (instructionSetIndex) => { this.setState(({ statusCheck }) => ({ statusCheck: { ...statusCheck, [index]: { ...statusCheck[index], isFetchingStatus: true, }, }, })); // other stuff... }

Also, I'm curious if lodash has anything like Ramda's assocPath, which is an immutable set, and would reduce the previous example to this:

checkInstructionSetStatus = async (instructionSetIndex) => { this.setState(R.assocPath(['statusCheck', index, 'isFetchingStatus'], true)); }

Not sure. Am toggling between review and training, so... not in a position to Google this ATM.

I am going to leave this the way it is. setState is not synchronous, and I want isFetchingStatus to be set before checkInstructionSetStatus is called.

chrisdavies · 2018-05-01T17:17:24Z

src/core_plugins/kibana/server/tutorials/apm/on_prem.js

+  let apmIndexPattern = 'apm*';
+  try {
+    apmIndexPattern = server.config().get('xpack.apm.indexPattern');
+  } catch (error) {


Makes sense. I wonder if we could modify server.config().get to take an optional default value and avoid these try/catches everywhere. I've written these same try/catches elsewhere. Seems like something we can clean up somday, just a note...

elasticmachine · 2018-05-01T20:17:40Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-05-14T15:46:35Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-05-14T17:49:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

stacey-gammon

I had quite a bit of feedback regarding combining some of the props types that @chrisdavies first brought up. In order to work through my comments (and make sure they made sense), I ended up creating the changes myself, so if you decide to move forward with them, feel free to explore, or merge, them:

d96bf79

stacey-gammon · 2018-05-14T18:12:00Z

src/core_plugins/kibana/public/home/components/tutorial/instruction_set.js

+      statusMsg = this.renderStatusCheckMsg(msg, 'warning');
+    }
+
+    const checkStausStep = (


checkStatusStep (sp)

stacey-gammon · 2018-05-14T18:39:34Z

src/core_plugins/kibana/public/home/components/tutorial/instruction_set.js

+  statusCheckState: PropTypes.string,
+  onStatusCheck: PropTypes.func.isRequired,
+  isCheckingStatus: PropTypes.bool,
+  hasStatusCheckFailed: PropTypes.bool,


I think I'm +1 with @chrisdavies on consolidating statusCheckState, isCheckingStatus and hasStatusCheckFailed into an enum (I think I'd use the same statusCheckState name) and also using PropTypes.oneOf. I think you'd need: notchecked, checking, success, failed. Also we may eventually want to distinguish between an error (e.g. the es query was malformed and didn't complete) vs the check completed but no hits were returned, and this sets us up better for that.

I think it'd be a little clearer, rather than trying to figure out the combination of everything. As it is now, someone could pass in statusCheckState of incomplete but hasStatusCheckFailed of true and that doesn't really make sense. If we use an enum I think we can prevent some weird combinations.

stacey-gammon · 2018-05-14T18:42:38Z

src/core_plugins/kibana/public/home/components/tutorial/tutorial.js

+
+    const statusCheck = instructions.instructionSets.map((instructionSet) => {
+      return {
+        hasStatusCheck: instructionSet.statusCheck ? true : false,


Could do hasStatusCheck: !!instructionSet.statusCheck (assuming instructionSet.statusCheck is not a boolean that would just convert it to one). Maybe my local linter is set up differently from the one that our ci uses, but this actually gets called out in my intellij:

stacey-gammon · 2018-05-14T18:50:37Z

src/core_plugins/kibana/public/home/components/tutorial/tutorial.js

@@ -77,23 +78,35 @@ export class Tutorial extends React.Component {
    }
  }

-  setParamDefaults = () => {
+  initInstructionsState = () => {


Style nit: all the () => {..} functions are supposed to end with ;. Guess it's not part of our linter, but it gets called out in my intellij. I think it's part of our style guide.... (could be wrong).

stacey-gammon · 2018-05-14T18:58:11Z

src/core_plugins/kibana/public/home/components/tutorial/tutorial.js

+      let statusCheckState = undefined;
+      let isCheckingStatus = false;
+      let hasFailed = false;
+      if (_.get(this.state, `statusCheck[${index}].hasStatusCheck`, false)) {


There any reason to use the lodash version here? Seems like ever instruction set should have hasStatusCheck variable.

Actually, why have a hasStatusCheck value on state at all? Can you get away with just checking instructionSet.statusCheck here?

stacey-gammon · 2018-05-14T19:05:47Z

src/core_plugins/kibana/public/home/components/tutorial/tutorial.js

+    const esHitsCheckConfig = _.get(instructions, `instructionSets[${instructionSetIndex}].statusCheck.esHitsCheck`);
+    const { hasFailed, isComplete } = await this.fetchEsHitsStatus(esHitsCheckConfig);
+
+    this.setState((prevState) => {


Any reason to ever call setState if there is no esHitsCheck? Maybe pull out the inner

+ if (!esHitsCheckConfig) { + return { hasFailed: false, isComplete: false }; + }

so we can skip the setState call if it doesn't exist.

stacey-gammon · 2018-05-14T19:16:16Z

src/core_plugins/kibana/public/home/components/tutorial/tutorial.js

+    });
+
+    if (response.status > 300) {
+      return { hasFailed: true, isComplete: false };


This also pushes me to think an enum would be easier to follow. I thought this would be a bad state, but I guess you are using incomplete plus failed to mean there was an error, rather than a successful check but with no data? Below it's also a bit confusing that it's broken into two states that will always be mutually exclusive.

elasticmachine · 2018-05-15T18:11:33Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

stacey-gammon

lgtm pending passing ci

elasticmachine · 2018-05-15T19:00:27Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-05-15T20:51:02Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

nreese · 2018-05-15T20:55:02Z

x-pack functional test failure

20:32:03    │ proc  [ftr]          └-> Reserved roles are not editable
20:32:03    │ proc  [ftr]            └-> "before each" hook: global before each
20:32:03    │ proc  [ftr]            │ info  Taking screenshot "/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-x-pack/kibana/x-pack/test/functional/screenshots/failure/security app Management Security navigation Reserved roles are not editable.png"
20:32:04    │ proc  [ftr]            │ info  Current URL is: http://localhost:5640/app/kibana#/management/security/roles/edit/kibana_dashboard_only_user?_g=()
20:32:04    │ proc  [ftr]            │ info  Saving page source to: /var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-x-pack/kibana/x-pack/test/functional/failure_debug/html/security app Management Security navigation Reserved roles are not editable.html
20:32:04    │ proc  [ftr]          └- ✖ fail: "security app Management Security navigation Reserved roles are not editable"
20:32:04    │ proc  [ftr]          │          Management
20:32:04    │ proc  [ftr]          │            Security
20:32:04    │ proc  [ftr]          │              navigation
20:32:04    │ proc  [ftr]          │                Reserved roles are not editable:
20:32:04    │ proc  [ftr]          │        Error: expected false to equal true
20:32:04    │ proc  [ftr]          │         at Assertion.assert (node_modules/expect.js/index.js:96:13)
20:32:04    │ proc  [ftr]          │         at Assertion.be.Assertion.equal (node_modules/expect.js/index.js:216:10)
20:32:04    │ proc  [ftr]          │         at Assertion.(anonymous function) [as be] (node_modules/expect.js/index.js:69:24)
20:32:04    │ proc  [ftr]          │         at Context.it (test/functional/apps/security/management.js:142:60)
20:32:04    │ proc  [ftr]          │         at <anonymous>
20:32:04    │ proc  [ftr]          │         at process._tickCallback (internal/process/next_tick.js:188:7)

jenkins, test this

elasticmachine · 2018-05-15T22:11:02Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* add statusCheck configuration to instructionSetSchema * add status check step to instruction set * track status check state * query elasticsearch for hits * display message when status check completes * clean up * use callout to display status check results * Updated status check * update tutorial snapshot * add jest tests for InstructionSet component * pass function as prop instead of wrapping in new function * refactor checkInstructionSetStatus * update snapshots that broke after rebase * Suggested changes (#24) * update jest test for statusCheckState prop enum * update tutorial snapshots

sorenlouv · 2018-05-15T22:38:19Z

Thanks for doing this @nreese !

* add statusCheck configuration to instructionSetSchema * add status check step to instruction set * track status check state * query elasticsearch for hits * display message when status check completes * clean up * use callout to display status check results * Updated status check * update tutorial snapshot * add jest tests for InstructionSet component * pass function as prop instead of wrapping in new function * refactor checkInstructionSetStatus * update snapshots that broke after rebase * Suggested changes (#24) * update jest test for statusCheckState prop enum * update tutorial snapshots

alexfrancoeur · 2018-05-16T07:35:25Z

Nice @nreese! Can this be used for beats / logstash tutorials as well? I imagine we could query for an index that is created from the module itself.

cc: @tsg

nreese force-pushed the status_check branch from 7ea98f5 to b59cdb4 Compare April 27, 2018 17:47

nreese requested review from chrisdavies and stacey-gammon April 30, 2018 14:06

nreese added release_note:enhancement :Sharing v7.0.0 v6.4.0 labels Apr 30, 2018

stacey-gammon reviewed May 1, 2018

View reviewed changes

nreese force-pushed the status_check branch from dd1e57a to c105e58 Compare May 1, 2018 14:41

chrisdavies approved these changes May 1, 2018

View reviewed changes

nreese force-pushed the status_check branch from f4ee871 to e95f043 Compare May 14, 2018 14:41

stacey-gammon reviewed May 14, 2018

View reviewed changes

stacey-gammon approved these changes May 15, 2018

View reviewed changes

nreese added 7 commits May 15, 2018 13:36

add statusCheck configuration to instructionSetSchema

a537e51

add status check step to instruction set

07f88d5

track status check state

2976108

query elasticsearch for hits

4a59a28

display message when status check completes

84fab71

clean up

9742ccd

use callout to display status check results

48fe20a

sorenlouv and others added 8 commits May 15, 2018 13:36

Updated status check

a9e9b81

update tutorial snapshot

47ad831

add jest tests for InstructionSet component

e3523fd

pass function as prop instead of wrapping in new function

dbd57f8

refactor checkInstructionSetStatus

30245f1

update snapshots that broke after rebase

d5176f1

Suggested changes (#24)

08081cf

update jest test for statusCheckState prop enum

101a8d1

nreese force-pushed the status_check branch from 25fe6f8 to 101a8d1 Compare May 15, 2018 19:37

nreese added the Feature:Home Kibana home application label May 15, 2018

update tutorial snapshots

184fa39

nreese merged commit 6811044 into elastic:master May 15, 2018

nreese mentioned this pull request May 15, 2018

[6.x] Add status check to "add data" tutorials (#17732) #19097

Merged

nreese mentioned this pull request Jul 11, 2018

Detect if new beats / logstash data is being ingested #16277

Closed

Add status check to "add data" tutorials #17732

Add status check to "add data" tutorials #17732

Conversation

nreese commented Apr 16, 2018 • edited Loading

elasticmachine commented Apr 16, 2018

💔 Build Failed

elasticmachine commented Apr 27, 2018

💔 Build Failed

elasticmachine commented Apr 30, 2018

💔 Build Failed

elasticmachine commented Apr 30, 2018

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented May 1, 2018

💚 Build Succeeded

chrisdavies 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

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 May 1, 2018

💚 Build Succeeded

elasticmachine commented May 14, 2018

💔 Build Failed

elasticmachine commented May 14, 2018

💚 Build Succeeded

stacey-gammon 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

Choose a reason for hiding this comment

elasticmachine commented May 15, 2018

💔 Build Failed

stacey-gammon left a comment

Choose a reason for hiding this comment

elasticmachine commented May 15, 2018

💔 Build Failed

elasticmachine commented May 15, 2018

💔 Build Failed

nreese commented May 15, 2018

elasticmachine commented May 15, 2018

💚 Build Succeeded

sorenlouv commented May 15, 2018

alexfrancoeur commented May 16, 2018

nreese commented Apr 16, 2018 •

edited

Loading