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

[APM] Don't read APM index names directly from config #49620

Closed
1 of 2 tasks
sorenlouv opened this issue Oct 29, 2019 · 14 comments
Closed
1 of 2 tasks

[APM] Don't read APM index names directly from config #49620

sorenlouv opened this issue Oct 29, 2019 · 14 comments
Assignees
Labels
Team:APM All issues that need APM UI Team support Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.6.0

Comments

@sorenlouv
Copy link
Member

sorenlouv commented Oct 29, 2019

In #48079 APM added support for overriding index names defined in kibana.yml via the UI. This means that reading a config value directly from the config might give a stale result, since it's being overridden by the UI settings.

Instead of reading directly from the config file one should read both from the config file and the UI settings and merge the result. Fortunately this is already taken care of by getApmIndices.

Example: apm_oss.transactionIndices
Instead of getting apm_oss.transactionIndices via config.get('apm_oss.transactionIndices') one should instead get it like:

import { getApmIndices } from 'x-pack/legacy/plugins/apm/server/lib/settings/apm_indices/get_apm_indices'

const apmIndices = getApmIndices(server)
const transactionIndices = apmIndices['apm_oss.transactionIndices']

Fix in

@sorenlouv sorenlouv added [zube]: Inbox Team:APM All issues that need APM UI Team support Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services labels Oct 29, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

@jasonrhodes
Copy link
Member

@elastic/apm-ui hey thanks for this heads up! I'm trying to use this in our NP shim layer now, where we will no longer have access to the server object. Can we talk through how we could make that possible?

@dgieselaar
Copy link
Member

dgieselaar commented Nov 1, 2019

@jasonrhodes After #49455 lands, server will no longer be required, but as it stands, getApmIndices would still need the APM configuration object. Not sure what the right solution is, the infra plugin could make APM an optional dependency, or we could expose an API endpoint.

@jasonrhodes
Copy link
Member

@dgieselaar I think the right solution will be for APM to return "getIndices" from its setup method, so that we can just call that directly. That is the place for plugins to share a contract, with types, as I understand it.

@jasonrhodes
Copy link
Member

I'm going to shim that method in a fake APM plugin for now, to get it working, and we can hammer out the details. :D

@ogupte
Copy link
Contributor

ogupte commented Nov 13, 2019

Also once #49994 merges, the server param will change to a single object param: { config: KibanaConfig, savedObjectsClient: ScopedSavedObjectsClient } https://github.com/elastic/kibana/pull/49994/files#diff-c8001ab8253c76efb10e2b90d98e2e36R60-R63. So It might be best to hold off until both #49455 and #49994 are merged.

@smith
Copy link
Contributor

smith commented Nov 13, 2019

Labelling as blocked since #49455 is blocked.

@dgieselaar
Copy link
Member

I've removed the blocked label from #49455, and will start work on this based off of that branch.

@dgieselaar
Copy link
Member

I'm not 100% sure whether we can fix the tutorial. It's all synchronous stuff over there. I mean, we can fix it by changing all the registerTutorial stuff to support async, but is it worth it at that point? I would imagine people would follow any tutorial before they start configuring UI indices.

@sorenlouv
Copy link
Member Author

I would imagine people would follow any tutorial before they start configuring UI indices

Yeah, I'm okay calling it an edge case. Additionally the tutorials might be revamped in the not so distant future so might be wasted work if we do it now.

@dgieselaar
Copy link
Member

I'd like to scope this ticket to exposing an getAPMIndices() method from the NP APM plugin contract, and then leave it up to infra to use it, WDYT @sqren? Looks like it requires quite a bit of plumbing from the infra side, e.g. a dependency on the APM NP plugin in a legacy infra plugin, and passing the APM contract (or part of it) through a few layers until we're at hasAPMData.

@sorenlouv
Copy link
Member Author

@dgieselaar SGTM 👍

@dgieselaar
Copy link
Member

Closing in favor of #50224 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:APM All issues that need APM UI Team support Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.6.0
Projects
None yet
Development

No branches or pull requests

7 participants