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

Migrate /api/settings route to Kibana platform #76416

Closed
pgayvallet opened this issue Sep 1, 2020 · 6 comments · Fixed by #77554
Closed

Migrate /api/settings route to Kibana platform #76416

pgayvallet opened this issue Sep 1, 2020 · 6 comments · Fixed by #77554
Assignees
Labels
Feature:Legacy Removal Issues related to removing legacy Kibana Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Monitoring Stack Monitoring team

Comments

@pgayvallet
Copy link
Contributor

The /api/settings route, that is currently in the legacy xpack_main plugin, is still used and needs to be migrated to the Kibana Platform

export function settingsRoute(server, kbnServer) {
server.route({
path: '/api/settings',
method: 'GET',
async handler(req) {

The endpoint is consumed by MetricBeat, and is an undocumented public API.

First question is: where should this move to? Should it move to the monitoring plugin, or should it be moved to something more 'generic'. Issue with this second option is that we don't currently have an equivalent to xpackMain in xpack plugins. Should we create one? Should we just create a 'settings' plugin?

Second question, regarding the data format

let settings = await settingsCollector.fetch(callCluster);
if (!settings) {
settings = settingsCollector.getEmailValueStructure(null);
}
const uuid = await getClusterUuid(callCluster);
const snapshotRegex = /-snapshot/i;
const config = server.config();
const status = kbnServer.status.toJSON();
const kibana = {
uuid: config.get('server.uuid'),
name: config.get('server.name'),
index: config.get('kibana.index'),
host: config.get('server.host'),
port: config.get('server.port'),
locale: config.get('i18n.locale'),
transport_address: `${config.get('server.host')}:${config.get('server.port')}`,
version: kbnServer.version.replace(snapshotRegex, ''),
snapshot: snapshotRegex.test(kbnServer.version),
status: get(status, 'overall.state'),
};

Most infos can be retrieved with KP APIs, but:

locale: config.get('i18n.locale')

This is currently not accessible via any core API. How should this be retrieved from a plugin.

const status = kbnServer.status.toJSON(); 
status: get(status, 'overall.state'),

@joshdover Will the KP status API be close to the legacy one? Can the new status API format be 'converted' to the legacy one?

@chrisronline Can you find out which properties of the overall status MetricBeat is consuming?

cc @elastic/kibana-platform @elastic/stack-monitoring-ui

@pgayvallet pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Monitoring Stack Monitoring team Feature:Legacy Removal Issues related to removing legacy Kibana labels Sep 1, 2020
@joshdover
Copy link
Contributor

joshdover commented Sep 1, 2020

First question is: where should this move to? Should it move to the monitoring plugin, or should it be moved to something more 'generic'. Issue with this second option is that we don't currently have an equivalent to xpackMain in xpack plugins. Should we create one? Should we just create a 'settings' plugin?

It's unclear to me from the code what this endpoint accesses that isn't available on the /api/stats endpoint? I think this may only be in x-pack for historical reasons. Is there anything it accesses that isn't available in OSS? It indirectly depends on a specific usage collector, but I don't believe it actually needs to know about that collector in order to report it's data.

I'm working on moving the /api/stats endpoint now. I think we can probably move this to the same location in OSS if my assumptions here are correct. I'm happy to pick this up as a follow-up.

locale: config.get('i18n.locale')

This is currently not accessible via any core API. How should this be retrieved from a plugin.

Yes, I'm going to have to solve this in the /api/stats work as well.

const status = kbnServer.status.toJSON(); 
status: get(status, 'overall.state'),

@joshdover Will the KP status API be close to the legacy one? Can the new status API format be 'converted' to the legacy one?

The HTTP API is not changing by default in 7.x, but will have a slightly different format. That said, the overall.state property is just the legacy level (green, yellow, or red) and can be easily sourced from the new status levels. Monitoring added a similar conversion here: #75806. I'm doing something similar in the work I have in progress, so we can use the same mechanism for this endpoint too.

@pgayvallet
Copy link
Contributor Author

It's unclear to me from the code what this endpoint accesses that isn't available on the /api/stats endpoint? I think this may only be in x-pack for historical reasons

@chrisronline do you have any idea about that one?

@mshustov
Copy link
Contributor

mshustov commented Sep 2, 2020

locale: config.get('i18n.locale')

The platform does know the current locale

@chrisronline
Copy link
Contributor

It's unclear to me from the code what this endpoint accesses that isn't available on the /api/stats endpoint? I think this may only be in x-pack for historical reasons. Is there anything it accesses that isn't available in OSS? It indirectly depends on a specific usage collector, but I don't believe it actually needs to know about that collector in order to report it's data.

The main difference is the /api/settings endpoint features an optional xpack path which will include the configured email address for receiving cluster alerts. This setting will go away in 8.0 though.

The kibana structure is also slightly different, but not for any good reason.

We need to keep the API around during 7.x, but I don't see any reason why we can't merge the two for 8.0, if desired.

@pgayvallet
Copy link
Contributor Author

The main difference is the /api/settings endpoint features an optional xpack path which will include the configured email address for receiving cluster alerts

Oh right, there are some differences between master and 7.x regarding that:

export async function getDefaultAdminEmail(config: MonitoringConfig) {
return config.cluster_alerts.email_notifications.email_address || null;
}

https://github.com/elastic/kibana/blob/7.x/x-pack/plugins/monitoring/server/kibana_monitoring/collectors/get_settings_collector.ts#L30

Both that code, the collector and the KIBANA_SETTINGS_TYPE collector key are currently in xpack code, so I guess that

I'm working on moving the /api/stats endpoint now. I think we can probably move this to the same location in OSS if my assumptions here are correct

is not an option?

I had to create the xpack_legacy plugin in 7.x to keep the xPack:defaultAdminEmail uiSetting that is no longer registered in master.

https://github.com/elastic/kibana/blob/7.x/x-pack/plugins/xpack_legacy/server/plugin.ts

Maybe it makes sense to add this plugin also in master and move this endpoint registration here?

@chrisronline
Copy link
Contributor

@pgayvallet I think that makes sense. I don't mind this living in a "legacy" location, and then plan to remove it in 8.0 and update Metricbeat to stop calling it, and instead call the /api/stats endpoint and use the kibana key from the response and index it into the kibana_settings monitoring document

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Legacy Removal Issues related to removing legacy Kibana Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Monitoring Stack Monitoring team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants