[home] Include ability to publish kibana saved objects from add data tutorial

[nreese]

fixes #17803

cc @makwarth @formgeist

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[sorenlouv]

If you don't need the constructor for anything else, you can set the shorthand:

export class SavedObjectsInstaller extends React.Component {
    state = {
        isInstalling: false,
        installStatus: INCOMPLETE,
    };
}

[sorenlouv]

For consistency installStatus should be a boolean like isInstalling (and named isComplete).

[sorenlouv]

If status only has two modes, I think it would be better to make it a boolean.

[sorenlouv]

I prefer const since it gives the assurance that this variable won't be reassigned later. If you use a ternary you can avoid let.

[sorenlouv]

Instead of having two different checks for error (errors.length === 0 and errors.length > 0) maybe just have one const hasError = errors.length > 0.

This way they won't somehow come out of sync in a future change.

[sorenlouv]

I think we made a mistake by coupling rest clients to Angular in the past. Instead they should have been framework-agnostic static functions.

If we keep coupling new code (in this case React) to Angular we make it harder to switch away from Angular, and increase technical debt.

It's easier to use an existing client but I don't think the alternative is much more difficult. You don't have to re-implement the entire savedObjectsClient - just create a starting point with the endpoints you need.

Afaict this is all you need:

export function bulkCreate(savedObjects, { overwrite }) {
  return kfetch({
    pathname: `/api/saved_objects/_bulk_create`,
    method: 'POST',
    query: {
      overwrite
    },
    body: JSON.stringify(savedObjects)
  });
}

[sorenlouv]

@epixa Do we already have a strategy for consuming Angular services from outside Angular - and how to migrate them? I've already spent a fair amount of time de-angularizing some services and it would be beneficial if everyone were aligned. If we don't have anything I'm open for doing a write-up of the approach I've used, and how I see us moving forward.

[nreese]

I do not think this is a problem and is exactly how we are interacting with saved objects in other react components. From react's perspective, it is just an injected function. Eventually, the savedObjectClient will be decoupled from angular but all saved object interaction should go through the savedObjectClient.

[sorenlouv]

but all saved object interaction should go through the savedObjectClient

Maybe I don't know enough about the savedObjectClient. What does it provide wrt bulkCreate, that a static method can't do?

[nreese]

The savedObjectClient converts the results into savedObject classes and camel cases the object keys. But more importantly, it puts all calls to the saved object API in a single place so if the API ever changes, only the savedObjectClient has to be updated instead of hunting down lots of segregated fetches.

[sorenlouv]

But more importantly, it puts all calls to the saved object API in a single place so if the API ever changes, only the savedObjectClient has to be updated instead of hunting down lots of segregated fetches

Totally agree. IMHO it is okay temporarily to have two "clients": the old Angular client, and a new framework agnostic service. Over time the Angular client could even call the framework agnostic client, and eventually the Angular client would be dropped. That's just my view, but interested to hear what other ideas are floating around (ping @epixa ;) )

[sorenlouv]

Btw. I think it's fine to leave it as is. Just my opinion on how to do it going forward.

[epixa]

Not in this PR, but I'd rather just remove the angular dependency from the saved object client we have today. It shouldn't be terribly difficult, we'd just need to update any references to the saved object client from angular code to invoke $scope.$apply() after the call.

[sorenlouv]

Is this just for debugging purposes? I assume this should be dynamically generated, so that when the mapping changes this doesn't get out of sync.

[nreese]

Storing the index pattern saved object like this is on purpose. If the mappings change between versions then this will need to be updated - just like the other saved objects will need to be updated in the event that field names change and so on.

[sorenlouv]

True, we still need to update the dashboards if a field is renamed or removed. But up until now we've been able to make backwards compatible changes to the schema (like adding a new field) that didn't require us to update anything in Kibana.

What is the advantage of storing the static object, instead of creating it dynamically?

[nreese]

The advantage is being able to link the other saved objects directly to the index-pattern by id. How does the import work now, how do the saved objects link to the index-pattern? Are the users prompted with a list of index-patterns that match the index-pattern title?

[sorenlouv]

The user might access APM UI (which also needs the index pattern) before opening the Getting Started Guide. We therefore need a single solution for creating the index pattern, so we don't end up with duplicate saved objects files that needs to be updated whenever apm-server schema changes.

I outlined three different solutions here: elastic/apm-server#991

If you have some alternative solutions I'm all ears :)

[makwarth]

@nreese You might be right. The thinking behind having two buttons ["Import index pattern", "Import index pattern and dashboards"] is that users will need to manually update the index pattern whenever they update their version of the stack. However, clicking "Import index pattern and dashboards" will also overwrite existing visualizations (AFAIK, correct me if I'm wrong), which can be annoying.
But, we might be overthinking this. Thoughts?

[makwarth]

Bit more context: The reason for mentioning "Index pattern" specifically is because we have this call out to the user in the UI: #20077
Having said that, I like what you did with abstracting all the details away by writing "Import Kibana objects". Maybe that's much better. We can have the details on a documentation page, or something. @formgeist @sqren Thoughts?

[formgeist]

+1 for just having one option and calling it "Import Kibana objects".

[sorenlouv]

Fine with me. If this causes issues for the users we can iterate on it.

[makwarth]

Agreed, let's go with one button. Thanks for input @nreese.

Copy suggestion for the setup instructions step:

Title:
Kibana objects

Description:
Imports index pattern, visualizations and pre-defined dashboards. Index pattern is required for some features in the APM UI.

Button copy:
Load/Import Kibana objects

[sorenlouv]

Instead of disabling eslint, I think the JSON should be moved to JSON-files. That will also make it easier to update.

[nreese]

These can not move to a json file since they need javascript logic to set the apm index pattern title

[sorenlouv]

Perhaps this comment was meant for get_index_pattern_saved_object.js?

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[gchaps]

@nreese @alexfrancoeur I do like being more specific as in "Load Kibana dashboards and visualizations" However, if I read things correctly the objects can also be index patterns and searches, so that title isn't quite accurrate. How about:

Load Kibana saved objects

Add saved searches, visualizations, dashboards, and index patterns.

The other steps don't start with "Click the button" so I'd eliminate that here. Also, no need to init cap the objects

[sorenlouv]

From a general point of view, it makes sense to keep the copy about "objects" since other Getting Started Guides might want to import searches, visualizations etc (like @gchaps mentions).

The use case for APM however is pretty specific: we are interested in importing dashboards and index patterns - preferably as separate flows (but a single flow might work also).

We mention the Getting Started Guide and link to it from the APM ui when using the new Query Bar:

There's no APM index pattern available. 
To use the Query bar, please import the APM index pattern via the Setup Instructions <link to setup instructions>

And also for creating ML jobs:

There's no APM index pattern available. 
To create a job, please import the APM index pattern via the Setup Instructions <link to setup instructions>

Would be great if the terminology in APM ui matched the terminology in the APM Getting Started Guide, so the user won't be confused when clicking the link.

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[sorenlouv]

Just to follow up on this: we can either change the copy in APM UI to be about "objects" instead of "index patterns", or vice versa for the Getting Started Guide.

@gchaps what do you think?

[sorenlouv]

Current copy in Getting Started Guide for reference

Button:

Load/Import Kibana objects

Potential warning:

5 of 9 objects already exist. Click 'Confirm overwrite' to import and overwrite existing objects. 
Any changes to the objects will be lost.

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[gchaps]

@sqren I lean toward using "index patterns and dashboards" in the Getting Started because the APM use case is specific to those objects. There's room in the GS for that text and its close to what's in the UI. If you feel it needs to b an exact match, then just use "index patterns".

[nreese]

@gchaps @sqren Where did we end up with this conversation? What should the copy be for this feature?

[gchaps]

@sqren and I agree: "Load Kibana index patterns and dashboards"

[stacey-gammon]

Have you tried just doing testSubjects.click? The move logic is internal:

    async click(selector, timeout = defaultFindTimeout) {
      log.debug(`TestSubjects.click(${selector})`);
      return await retry.try(async () => {
        const element = await this.find(selector, timeout);
        await remote.moveMouseTo(element);
        await element.click();
      });
    }

[stacey-gammon]

Two awaits here. Also, have you tried just extending the timeout? You can do something like:

await testSubjects.exists('loadSavedObjects_success', 10000) if the default timeout of 1 second is not long enough.

Another option is to just do await testSubjects.find('loadSavedObjects_success');

That should have an internal retry since it assumes it's supposed to exist:

    async _ensureElementWithTimeout(timeout, getElementFunction) {
      try {
        const remoteWithTimeout = remote.setFindTimeout(timeout);
        return await retry.try(async () => {
          const element = await getElementFunction(remoteWithTimeout);
          // Calling any method forces a staleness check
          element.isEnabled();
          return element;
        });
      } finally {
        remote.setFindTimeout(defaultFindTimeout);
      }
    }

[stacey-gammon]

nm, I see you already got rid of the second await in a follow up commit.

cc @spalger - I think this is a deficiency to having non-squashed commits. How do you keep up with all the changes from commit to commit to commit?

For instance I reviewed before this commit. I wanted to see the functional tests added so I went to that commit and added the comment. But the next commit fixed it. So what would your flow be? Start at commit x and review each one, x -> x + 1 -> x + 2 ... ? What if an x + n commit fixed something you commented on prior? Just go back and delete it?

Or do you review all commits from one commit onward but not the pull merges? Doesn't seem possible to do this in the git UI. It's either review all changes, or review commit by commit.

[stacey-gammon]

I signed in as a read only user and got a spinning button but no error message:

If this is a legit bug, can you add tests for it? Think a jest test would cover it, if bulkCreate throws an error. Probably good to add one if you simulate it to return 409 errors too (have not walked through that case manually yet).

(as an aside, my license is expired, so it's possible that is causing an issue too... I'll check with a non expired license)

[sorenlouv]

With "async/await everything" I've started to write:

const bulkCreateMock = async () => { 
  throw new Error('simulated bulkRequest error') 
}

It is not much shorter but I've come to like it. Use it if you want :)

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[chrisdavies]

LGTM.

Looked over the code. Only one real comment.

I loaded the Kibana objects, and it seemed to work fine.

[chrisdavies]

Might be nice to just have a config.getOrDefault(TITLE_KEY, DEFAULT_TITLE) method. Seems like I've seen this same conditional in lots of places, not just in this PR.

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[sorenlouv]

Thanks for doing this @nreese !!