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] Make lightweight client for reading APM event data #67397

Closed
sorenlouv opened this issue May 26, 2020 · 12 comments · Fixed by #82724
Closed

[APM] Make lightweight client for reading APM event data #67397

sorenlouv opened this issue May 26, 2020 · 12 comments · Fixed by #82724
Assignees
Labels
Team:APM All issues that need APM UI Team support technical debt Improvement of the software architecture and operational architecture v7.11.0

Comments

@sorenlouv
Copy link
Member

sorenlouv commented May 26, 2020

An increasing number of teams need to read APM data (transactions, spans, errors etc.). Until now we’ve told them to read directly from the appropriate index with the caveat that these are configurable. Additionally they need to take into account that users can overwrite this via the runtime settings.

I propose that we create a lightweight client for consuming APM event data that will abstract this away.
Bonus points if we can add support for spaces (whatever that means in an APM context).

Client for APM indices

apmClient.search({
  apmEvents: [ProcessorEvent.transaction]
  size: 100,
  query: {...},
  aggs: {...}
})

This is used for querying public (non system) APM indices and will use the current user's auth

The apmEvents will allows us to search the appropriate indices and inject a terms filter for processor.event like terms: { processor.event: ['transaction', 'span' ] }.

@sorenlouv sorenlouv added the Team:APM All issues that need APM UI Team support label May 26, 2020
@elasticmachine
Copy link
Contributor

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

@dgieselaar
Copy link
Member

Very much a ➕ on having a very thin wrapper around ES. Maybe we should just override index, rather than accepting only the body parameter. I'm also not sure about docTypes as a second parameter. Feels like it's trying to solve a problem that we might not have today?

@sorenlouv
Copy link
Member Author

sorenlouv commented May 27, 2020

I'm also not sure about docTypes as a second parameter. Feels like it's trying to solve a problem that we might not have today?

I don't think the current signature is pretty either so open to suggestions. But I don't see how we can get rid of it entirely. How would we know which index the user wants to query without it?

@cauemarcondes
Copy link
Contributor

Changing it to v7.9.0 because it is going to be used in the Observability Overview page. #68176

@dgieselaar
Copy link
Member

Thinking about this some more, I wonder how it will work out if we start using metrics. e.g. the consumer of the API would need to check whether metrics should be queried, and then configure the appropriate indices, processor.event query and the field names for the aggregation. Maybe we do need a higher-level abstraction?

@cauemarcondes cauemarcondes removed their assignment Jun 12, 2020
@sorenlouv sorenlouv added technical debt Improvement of the software architecture and operational architecture Team:APM All issues that need APM UI Team support and removed v7.9.0 Team:APM All issues that need APM UI Team support labels Jul 8, 2020
@sorenlouv
Copy link
Member Author

I've added this to 7.11 since it's a requirement for moving the CSM app out of APM.
@dgieselaar With your latest changes to the elasticsearch client in apm, how difficult do you imagine this'll be? It seems pretty straight-forward to me but I might be missing some things.

@dgieselaar
Copy link
Member

@sqren I don't think it will be difficult. We'd need to remove the dependency on APMRequestHandlerContext in x-pack/plugins/apm/server/lib/helpers/create_es_client/create_apm_event_client/index.ts, and make some other small changes.

@sorenlouv
Copy link
Member Author

Great! I'm thinking we might want to do it after FF so it's ready at the beginning of the 7.11 development cycle.

@shahzad31
Copy link
Contributor

@sqren @dgieselaar any progress on this? are we on plan for 7.11 for this?

@sorenlouv
Copy link
Member Author

@shahzad31 It's planned for 7.11 but we haven't started it yet. I'll make sure it gets prioritised and done sooner rather than later.

@dgieselaar
Copy link
Member

Should we rescope this issue to only concern the APM event client? We can create an issue for the other clients (but I think it might just lying around in the backlog for a long while).

@sorenlouv
Copy link
Member Author

@dgieselaar sgtm. Don't worry about creating new issues, I agree they'll just be clutter.

@dgieselaar dgieselaar changed the title [APM] Make lightweight client for reading APM data [APM] Make lightweight client for reading APM event data Nov 5, 2020
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 technical debt Improvement of the software architecture and operational architecture v7.11.0
Projects
None yet
6 participants