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

[WIP][APM] APM client #68264

Closed
wants to merge 1 commit into from
Closed

Conversation

cauemarcondes
Copy link
Contributor

closes #67397

@cauemarcondes cauemarcondes requested a review from a team June 4, 2020 15:05
@kibanamachine
Copy link
Contributor

💔 Build Failed

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

search: async ({ apmIndices, body }: Search) => {
const indices = apmIndices.map((index: Index) => indexMap[index]);
// TODO: callAsCurrentUser isn't available here. How to get it?
const { callAsInternalUser } = core.elasticsearch.legacy.client;
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to use RequestHandlerContext like we do in

async function getParamsForSearchRequest(
context: APMRequestHandlerContext,
params: ESSearchRequest,
apmOptions?: APMOptions
) {
const { uiSettings } = context.core;
const [indices, includeFrozen] = await Promise.all([
getApmIndices({
savedObjectsClient: context.core.savedObjects.client,
config: context.config,
}),
uiSettings.client.get('search:includeFrozen'),
]);
// Get indices for legacy data filter (only those which apply)
const apmIndices = Object.values(
pickKeys(
indices,
'apm_oss.sourcemapIndices',
'apm_oss.errorIndices',
'apm_oss.onboardingIndices',
'apm_oss.spanIndices',
'apm_oss.transactionIndices',
'apm_oss.metricsIndices'
)
);
return {
...addFilterForLegacyData(apmIndices, params, apmOptions), // filter out pre-7.0 data
ignore_throttled: !includeFrozen, // whether to query frozen indices or not
};
.

I think for querying public apm indices (not system indices) the current user should be used. For querying internal indices like .apm-agent-configuration or .apm-custom-links the internal user should be used.
Thinking more about this, it should probably be separate clients.

So the following client should always use the current user, and should not be used for querying system indices:

apmClient.search({
  apmIndices: ['transaction', 'span'],
  size: 100,
  query: {...},
  aggs: {}
})

The following client should always use the internal user, but only be used to querying system indices

apmInternalClient.search({
  apmIndices: ['.apm-agent-configuration', '.apm-custom-links'],
  size: 100,
  query: {...},
  aggs: {}
})

Copy link
Member

Choose a reason for hiding this comment

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

The "normal" apmClient should filter out legacy data addFilterForLegacyData and handle frozen indices. apmInternalClient should not.

Copy link
Member

Choose a reason for hiding this comment

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

Currently we have one client that all requests are made with. I think we should go away from that thinking and instead have multiple smaller clients, eg apmClient, apmInternalClient, mlClient (until they ship their own) etc.

Copy link
Member

Choose a reason for hiding this comment

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

It would be great if you can update es_client to reflect this change.

Copy link
Member

@sorenlouv sorenlouv Jun 5, 2020

Choose a reason for hiding this comment

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

I think you can simplify apmClient a lot since it should only have a search method (no delete, index etc).

@cauemarcondes cauemarcondes deleted the apm-client branch September 17, 2020 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[APM] Make lightweight client for reading APM event data
3 participants