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

[Integrate Profiling with APM] Navigate from the transaction details view into the Profiling #159686

Conversation

cauemarcondes
Copy link
Contributor

@cauemarcondes cauemarcondes commented Jun 14, 2023

  • The new profiling items will be only visible when the profiling plugin has been already installed. Otherwise, these are going to be hidden.
  • The profiling plugin exposes three new Locators to facilitate the navigation to the Flamegraph, TopN functions and Stacktraces pages.
  • Add new badge property on the section component
Screenshot 2023-06-14 at 1 55 09 PM
Screen.Recording.2023-06-14.at.1.06.42.PM.mov
Screen.Recording.2023-06-14.at.1.06.19.PM.mov
flamegraph.mov

@cauemarcondes cauemarcondes requested review from a team as code owners June 14, 2023 12:17
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Jun 14, 2023
@elasticmachine
Copy link
Contributor

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

@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!)

@cauemarcondes cauemarcondes changed the title [Integrate Profiling with APM] Navigate from a service’s view into the Profiling [Integrate Profiling with APM] Navigate from a transaction details view into the Profiling Jun 14, 2023
@cauemarcondes cauemarcondes changed the title [Integrate Profiling with APM] Navigate from a transaction details view into the Profiling [Integrate Profiling with APM] Navigate from the transaction details view into the Profiling Jun 14, 2023
@formgeist
Copy link
Contributor

@cauemarcondes I think there's been some slight degradations on the Investigate links, since they appear massive right now. Can we please reduce the size of the links and changing the style to primary?

Here's the changes to the default props to the EuiListGroup component as applied in the Investigate menu.

export function SectionLinks({ children, ...props }: { children?: ReactNode } & EuiListGroupProps) {
  return (
    <EuiListGroup {...props} size={'s'} color={'primary'} flush={true} bordered={false}>
      {children}
    </EuiListGroup>
  );
}
CleanShot 2023-06-14 at 14 43 35@2x

@cauemarcondes
Copy link
Contributor Author

@formgeist I updated the description with the changes you suggested. Thanks for that.

Copy link
Contributor

@gbamparop gbamparop left a comment

Choose a reason for hiding this comment

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

APM changes LGTM!

@gbamparop
Copy link
Contributor

cc @boriskirov

@cauemarcondes cauemarcondes requested a review from a team as a code owner June 26, 2023 10:02
Copy link
Contributor

@Ikuni17 Ikuni17 left a comment

Choose a reason for hiding this comment

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

limits.yml

Copy link
Contributor

@thomasdullien thomasdullien left a comment

Choose a reason for hiding this comment

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

LGTM :)

@cauemarcondes cauemarcondes enabled auto-merge (squash) July 3, 2023 13:28
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
apm 1460 1461 +1
profiling 166 169 +3
total +4

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
profiling 16 19 +3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
apm 3.6MB 3.6MB +1.3KB
profiling 294.9KB 289.7KB -5.2KB
total -3.9KB

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
profiling 0 3 +3

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
observabilityShared 41.6KB 41.9KB +358.0B
profiling 14.6KB 21.1KB +6.5KB
total +6.8KB
Unknown metric groups

API count

id before after diff
profiling 16 19 +3

ESLint disabled line counts

id before after diff
enterpriseSearch 14 16 +2
securitySolution 410 414 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 15 17 +2
securitySolution 489 493 +4
total +6

History

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

Copy link
Member

@jasonrhodes jasonrhodes left a comment

Choose a reason for hiding this comment

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

I've added some questions/concerns about dependency introductions.

Copy link
Member

Choose a reason for hiding this comment

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

I like locators as an abstraction. I don't like how they introduce dependencies between observability apps (what happens if profiling wants to link to APM?).

Should we introduce a pattern for keeping locators in "observability-shared", separated by folders that could still be managed via our standard codeowners?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for raising this up. Let me see what I can move to the observability-shared.

Copy link
Member

Choose a reason for hiding this comment

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

I don't want to unnecessarily hold this feature up to solve this, so if we need to address this outside of this PR, that's fine. But we should talk about what this pattern should look like. APM already depends on the "infra" plugin and uses its locators (#158365) but the APM -> Infra plugin dependency causes lots of problems for us overall, imo.

Copy link
Member

Choose a reason for hiding this comment

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

My biggest worry is that the introduction of this dependency would lead us to start pulling other things in, and making it harder to disconnect that dependency in the future, which is the future I want us to aim for (no deps between obs UI plugins). I have a plan for how we might get around this, stay tuned.

Copy link
Member

Choose a reason for hiding this comment

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

We're likely going to want this exact thing in the Hosts UI view (cc: @neptunian) -- I don't want to abstract too early so we can copy/paste at first, but eventually we should consider whether it makes sense to have an ObservabilityContext we all share/use/load. I imagine that would need to live in the observability-shared plugin (similar to my comment about locators).

x-pack/plugins/apm/kibana.jsonc Show resolved Hide resolved
Copy link
Member

@jasonrhodes jasonrhodes left a comment

Choose a reason for hiding this comment

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

Caue and I talked a bit about the APM -> Profiling dependency and decided it's not something we should block this PR for, but that we should sync up ASAP and create an issue to see if we can find a way to remove that dependency by leveraging the observability_shared plugin in a couple different ways.

@cauemarcondes cauemarcondes merged commit d118fb4 into elastic:main Jul 6, 2023
22 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 6, 2023
@cauemarcondes cauemarcondes deleted the profiling-apm-int-service-instances-table branch July 6, 2023 17:00
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:enhancement Team:APM All issues that need APM UI Team support v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants