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

[Monitoring] Kibana settings api #21100

Merged

Conversation

chrisronline
Copy link
Contributor

@chrisronline chrisronline commented Jul 23, 2018

Resolves #20903

Summary

This PR exposes an api that includes nearly the same information from the kibana_settings monitoring docs through an api endpoint for consumption through Metricbeat.

The api endpoint is: /api/settings and the response looks like:

{
  "cluster_uuid": "iB7C1tpwSamLc-ye8qTEwA",
  "settings": {
    "kibana": {
      "uuid": "5b2de169-2785-441b-ae8c-186a1936b17d",
      "name": "Elastic-MBP.local",
      "index": ".kibana",
      "host": "localhost",
      "transport_address": "localhost:5601",
      "version": "7.0.0-alpha1",
      "snapshot": false,
      "status": "green"
    }
  }
}

Testing

To test, you'll need to grab sample kibana_settings monitoring document:

POST .monitoring-kibana-6-*/_search
{
  "_source": "kibana_settings",
  "sort": {
    "timestamp": "desc"
  },
  "query": {
    "term": {
      "type": {
        "value": "kibana_settings"
      }
    }
  }
}

Compare the response from the above (within the kibana field) with the kibana field from the api response and ensure they match.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@ycombinator
Copy link
Contributor

ycombinator commented Jul 26, 2018

Functionally this LGTM. I tested it with elastic/beats#7664 and it works as expected. Will review code shortly.

const settings = await settingsCollector.fetch(callCluster);
const uuid = await getClusterUuid(callCluster);

console.log('uuid', uuid);
Copy link
Contributor

Choose a reason for hiding this comment

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

Debugging statement.

Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

Just a minor comment, and then LGTM.

@elasticmachine
Copy link
Contributor

💔 Build Failed

*/

import { wrap as wrapError } from 'boom';
import { getKibanaInfoForStats } from '../../../../../../../src/server/status/lib';
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like CI is failing on this line:

15:25:10    │ debg   error  [22:25:10.553] [fatal] Error: Cannot find module '../../../../../../../src/server/status/lib'
15:25:10    │ debg      at Function.Module._resolveFilename (module.js:547:15)
15:25:10    │ debg      at Function.Module._load (module.js:474:25)
15:25:10    │ debg      at Module.require (module.js:596:17)
15:25:10    │ debg      at require (internal/module.js:11:18)
15:25:10    │ debg      at Object.<anonymous> (/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-x-pack/kibana/build/kibana/node_modules/x-pack/plugins/xpack_main/server/routes/api/v1/settings.js:8:1)
...

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

}
});
} catch(err) {
req.log(['error'], err); // FIXME doesn't seem to log anything useful if ES times out
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this FIXME for addressing in this PR itself or a follow up PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. It's a comment copied over from the other api endpoint. Let's say it will be addressed in a follow up PR

Copy link
Member

Choose a reason for hiding this comment

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

I suspect the follow PR would need to address this problem everywhere

@chrisronline chrisronline changed the title [WIP] [Monitoring] Kibana settings api [Monitoring] Kibana settings api Jul 27, 2018
Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

Tested with elastic/beats#7664. LGTM.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@chrisronline chrisronline requested review from simianhacker and removed request for tsullivan July 31, 2018 14:35
@chrisronline
Copy link
Contributor Author

@simianhacker Have some cycles to review this for me?

Copy link
Member

@simianhacker simianhacker left a comment

Choose a reason for hiding this comment

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

LGTM... I had a question about the tests. Right now you're just making sure at least one cluster show's up but that doesn't really test if we support more then one.

}
});
} catch(err) {
req.log(['error'], err); // FIXME doesn't seem to log anything useful if ES times out
Copy link
Member

Choose a reason for hiding this comment

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

I suspect the follow PR would need to address this problem everywhere

.get('/api/settings')
.set('kbn-xsrf', 'xxx')
.expect(200);
expect(body.cluster_uuid.length > 0).to.eql(true);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be more then 1 if we expect multiple?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, good point!

@simianhacker
Copy link
Member

FYI... You can probably merge this once it's green.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@chrisronline chrisronline merged commit 871d283 into elastic:master Aug 7, 2018
@chrisronline chrisronline deleted the monitoring/kibana_settings_api branch August 7, 2018 17:12
chrisronline added a commit to chrisronline/kibana that referenced this pull request Aug 7, 2018
* Kibana settings api

* Use different version of this utility

* Adding settings api test

* Fix these tests

* Update test
chrisronline added a commit that referenced this pull request Aug 7, 2018
* Kibana settings api

* Use different version of this utility

* Adding settings api test

* Fix these tests

* Update test
@chrisronline
Copy link
Contributor Author

Backport:

6.x: cf03af7

ruflin pushed a commit to elastic/beats that referenced this pull request Aug 9, 2018
Resolves #7621.

Depends on elastic/kibana#21100.

X-Pack Monitoring of Kibana requires two types of documents in the `.monitoring-kibana-*` indices: `kibana_stats` and `kibana_settings`. We made Metricbeat's `kibana/stats` metricset index `kibana_stats` documents into `.monitoring-kibana-*` in #7525. This PR makes the same metricset index `kibana_settings` documents into `.monitoring-kibana-*`.
ycombinator added a commit to ycombinator/beats that referenced this pull request Aug 10, 2018
Resolves elastic#7621.

Depends on elastic/kibana#21100.

X-Pack Monitoring of Kibana requires two types of documents in the `.monitoring-kibana-*` indices: `kibana_stats` and `kibana_settings`. We made Metricbeat's `kibana/stats` metricset index `kibana_stats` documents into `.monitoring-kibana-*` in elastic#7525. This PR makes the same metricset index `kibana_settings` documents into `.monitoring-kibana-*`.

(cherry picked from commit 2af5ab9)
ruflin pushed a commit to elastic/beats that referenced this pull request Aug 13, 2018
Resolves #7621.

Depends on elastic/kibana#21100.

X-Pack Monitoring of Kibana requires two types of documents in the `.monitoring-kibana-*` indices: `kibana_stats` and `kibana_settings`. We made Metricbeat's `kibana/stats` metricset index `kibana_stats` documents into `.monitoring-kibana-*` in #7525. This PR makes the same metricset index `kibana_settings` documents into `.monitoring-kibana-*`.

(cherry picked from commit 2af5ab9)
return esArchiver.unload(archive);
});

it('should load multiple clusters', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

description should be different :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants