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

[Profiling] Prepare for inline stackframes #150401

Merged
merged 8 commits into from
Feb 7, 2023

Conversation

jbcrail
Copy link
Contributor

@jbcrail jbcrail commented Feb 7, 2023

Summary

This PR adds initial support for initial stackframes as described in elastic/prodfiler#2918.

It also adds tests and a minor refactor to account for the removal of synthetic source from stackframes (see elastic/prodfiler#2850).

For reviewers

For stackframes, the profiling stack is composed of multiple write paths into Elasticsearch and multiple read paths out of Elasticsearch:

  • there are three services that can write into Elasticsearch (APM agent, pf-elastic-collector, and pf-elastic-symbolizer).
  • there are also two ways to read from Elasticsearch (the profiling plugin in Elasticsearch, and a combination of search and mget calls).

This PR was written to handle all permutations of these paths. For those reviewers that wish to try the PR, please keep this in mind. I also wrote tests to handle these permutations.

Note: Future PRs will add full support for inline stackframes. At this time, we only read the first inlined stackframe since the UI does not support inline stackframes.

This addresses an error seen in elastic/prodfiler#2939 and was discussed
in elastic/prodfiler#2918.

Synthetic source was removed for stackframes in elastic/prodfiler#2850.
This addresses elastic/prodfiler#2918, which prepares for stackframe
inlining support.
Inline frame support requires that we can ingest array fields. However,
until inline frame support is supported fully both in all write paths
and the UI, we still need to handle stackframe fields that may also be
single values.
This was a result of an update from elastic/prodfiler#2850.
@jbcrail jbcrail added release_note:skip Skip the PR/issue when compiling release notes v8.7.0 labels Feb 7, 2023
@jbcrail jbcrail requested a review from a team as a code owner February 7, 2023 00:35
@rockdaboot
Copy link
Contributor

What doesn't work for me is getting interpreter symbols from the 8.6 MVP cluster. It seems to be a regression.

That is with switching Elasticsearch profiler plugin to off and adding

xpack.profiling.elasticsearch.hosts: https://profiling-next-stable-release.es.us-west2.gcp.elastic-cloud.com:9243
xpack.profiling.elasticsearch.username: <user>
xpack.profiling.elasticsearch.password: <password>

to config/kibana.dev.yml.

Screenshot_20230207_105626

@rockdaboot
Copy link
Contributor

rockdaboot commented Feb 7, 2023

I pushed a fix for this. @dgieselaar could you please have a look at it ?

Screenshot_20230207_114253

@rockdaboot rockdaboot enabled auto-merge (squash) February 7, 2023 11:16
@rockdaboot rockdaboot merged commit b42bb18 into elastic:main Feb 7, 2023
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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 Feb 7, 2023
@jbcrail jbcrail deleted the profiling-inline-frames branch February 7, 2023 15:50
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 v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants