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] Add data access plugin #162367

Merged
merged 23 commits into from
Aug 22, 2023
Merged

Conversation

sorenlouv
Copy link
Member

@sorenlouv sorenlouv commented Jul 20, 2023

Closes #161906
Related to: https://github.com/elastic/observability-dev/discussions/2787 (internal)

This add a new plugin apm_data_access that contains the APM query targets (indices to query for APM data).
This plugin can be consumed by apm and any other plugin, making it possible for other plugins to know about the configured APM query targets.

Example:

APM query targets can be specified in kibana[.dev].yml using xpack.apm.indices.{dataset}: some-index-* for instances:

xpack.apm.indices.transaction: apm-*

See all config options on: https://www.elastic.co/guide/en/kibana/current/apm-settings-kb.html#general-apm-settings-kb

Query targets can also be specified via the UI (and persisted in a saved object) via the settings page: /app/apm/settings/apm-indices

Retrieving the query targets
Query targets can be retrieved from other plugins via getApmIndices:

const apmIndices = await plugins.apmDataAccess.setup.getApmIndices(savedObjects.client); 

TODO:

@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@sorenlouv sorenlouv force-pushed the add-apm-data-access-plugin branch 3 times, most recently from dc9a777 to 04795e5 Compare July 20, 2023 23:50
@roshan-elastic
Copy link

Thanks so much for jumping on this @sqren @neptunian !

@sorenlouv sorenlouv force-pushed the add-apm-data-access-plugin branch from 951db70 to ac04548 Compare August 11, 2023 20:28
@sorenlouv sorenlouv marked this pull request as ready for review August 11, 2023 22:20
@sorenlouv sorenlouv requested a review from a team as a code owner August 11, 2023 22:20
@sorenlouv sorenlouv added release_note:skip Skip the PR/issue when compiling release notes v8.11.0 labels Aug 12, 2023
@sorenlouv
Copy link
Member Author

sorenlouv commented Aug 12, 2023

@neptunian This PR is now ready to test out. Can you try consuming the apmDataAccess plugin and see if you can retrieve the apm indices from infra or another plugin where you need it?

  1. Add apmDataAccess as required plugin

https://github.com/elastic/kibana/blob/ac0454896aebe072dbd831247a6c7bba4e4c75b7/x-pack/plugins/apm/kibana.jsonc#L11-L12

  1. Add ApmDataAccessPluginSetup and ApmDataAccessPluginStart to the plugin dependency types:

https://github.com/elastic/kibana/blob/ac0454896aebe072dbd831247a6c7bba4e4c75b7/x-pack/plugins/apm/server/types.ts#L80-L82

https://github.com/elastic/kibana/blob/ac0454896aebe072dbd831247a6c7bba4e4c75b7/x-pack/plugins/apm/server/types.ts#L105-L107

  1. Consume apmDataAccess

https://github.com/elastic/kibana/blob/146648a4688c5d98e99eb494284d54b5fbcb7f33/x-pack/plugins/apm/server/routes/apm_routes/register_apm_server_routes.ts#L113-L115

@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Aug 14, 2023
@elasticmachine
Copy link
Contributor

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

export const config: PluginConfigDescriptor<APMDataAccessConfig> = {
deprecations: ({ renameFromRoot }) => [
// deprecations due to removal of apm_oss plugin
renameFromRoot('apm_oss.transactionIndices', 'xpack.apm.indices.transaction', {
Copy link
Contributor

@gbamparop gbamparop Aug 14, 2023

Choose a reason for hiding this comment

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

Do we still have to use xpack.apm.* as the new keys here? Can these be replaced with xpack.apm_data_access.*?

Copy link
Member Author

@sorenlouv sorenlouv Aug 14, 2023

Choose a reason for hiding this comment

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

Yes, this will log a visible warning to the user if they are using apm_oss.transactionIndices. In that case they should be recommended to use xpack.apm.indices.transaction - not xpack.apm_data_access.indices.transaction.

For now apm_data_access is purely an implementation detail, and not something end users should know about (maybe that changes in the future but I'd like to keep this change invisible for now).

Copy link
Contributor

@neptunian neptunian Aug 15, 2023

Choose a reason for hiding this comment

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

@sqren I'm getting an error unless I use xpack.apm_data_access.indices.*

[config validation of [xpack.apm].indices]: definition for this key is missing

Copy link
Member Author

@sorenlouv sorenlouv Aug 15, 2023

Choose a reason for hiding this comment

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

@neptunian What's in your kibana.yml?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a copy paste of my remote oblt cluster config

Copy link
Member Author

Choose a reason for hiding this comment

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

xpack.apm.indices.sourcemap and xpack.apm.indices.onboarding were missing in the config definitions. I re-added them which should fix the issue

@sorenlouv sorenlouv force-pushed the add-apm-data-access-plugin branch from 2e019c7 to faf6c9e Compare August 14, 2023 13:06
@neptunian neptunian self-requested a review August 14, 2023 17:58
@sorenlouv sorenlouv force-pushed the add-apm-data-access-plugin branch from d839e7a to 85d88e4 Compare August 16, 2023 08:19
@@ -76,7 +74,8 @@ const ruleTypeConfig = RULE_TYPES_CONFIG[ApmRuleType.TransactionDuration];
export function registerTransactionDurationRuleType({
alerting,
ruleDataClient,
config$,
getApmIndices,
Copy link
Contributor

@yngrdyn yngrdyn Aug 16, 2023

Choose a reason for hiding this comment

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

Why not passing directly the indices instead of the function? Couldn't we get the indices on plugin start and save them as a resource for the routes?

Copy link
Member Author

@sorenlouv sorenlouv Aug 16, 2023

Choose a reason for hiding this comment

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

I considered it, but decided to load the indices lazily for two reasons:

  1. registerTransactionDurationRuleType is called when kibana starts. Retrieving the apm indicies at this point would mean that if the indices were update via APM Settings, the change would not be reflected until Kibana was restarted.

  2. Because registerTransactionDurationRuleType is called during startup it is called with the privileges of the internal kibana user and not with the privileges of the rule execution user. I don't think this is a problem in practice but it feels slightly wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

Copy link
Contributor

@yngrdyn yngrdyn left a comment

Choose a reason for hiding this comment

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

LGTM 📦

@sorenlouv sorenlouv enabled auto-merge (squash) August 20, 2023 23:44
@sorenlouv sorenlouv force-pushed the add-apm-data-access-plugin branch 2 times, most recently from e3aa6ee to 60cba7b Compare August 21, 2023 09:37
@sorenlouv sorenlouv added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:skip Skip the PR/issue when compiling release notes labels Aug 21, 2023
@sorenlouv sorenlouv force-pushed the add-apm-data-access-plugin branch 2 times, most recently from f8ea7ef to af8bb34 Compare August 21, 2023 17:00
@sorenlouv sorenlouv force-pushed the add-apm-data-access-plugin branch from af8bb34 to a8c4e9c Compare August 22, 2023 07:57
@sorenlouv sorenlouv merged commit 7df1cee into elastic:main Aug 22, 2023
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
apm 48 29 -19
apmDataAccess - 9 +9
total -10

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
apm 120 118 -2
Unknown metric groups

API count

id before after diff
apm 48 29 -19
apmDataAccess - 9 +9
total -10

ESLint disabled line counts

id before after diff
apmDataAccess - 1 +1

Total ESLint disabled count

id before after diff
apmDataAccess - 1 +1

History

  • 💔 Build #152031 failed af8bb345fa7cc7f380cd1f0ba28c23f81944edcc
  • 💔 Build #151946 failed f8ea7ef843b529f42c63290c7b6c39c5c7e4990d
  • 💚 Build #151775 succeeded 60cba7b8c3b1ad2df9777543571800475bb78150
  • 💔 Build #151724 failed 8f3abab9d44614b78e67a85f0492cbc10be7d0a0
  • 💔 Build #150990 failed 2df92fe6f1c664b1ec72f3154d637dd297e54104

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 22, 2023
@sorenlouv sorenlouv deleted the add-apm-data-access-plugin branch August 22, 2023 13:50
neptunian added a commit that referenced this pull request Sep 11, 2023
#164750)

Closes #164695

Uses [apm_data_access
plugin](#162367) in place of
statically defined indices to extract services

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Kevin Lacabane <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support v8.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create APM data access plugin
9 participants