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.

Sign up for GitHub

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

Jump to bottom

[APM] Breakdown Top dependencies API #211441

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

[APM] Breakdown Top dependencies API #211441

wants to merge 19 commits into from

Conversation

cauemarcondes
Copy link
Contributor

@cauemarcondes cauemarcondes commented Feb 17, 2025 •
edited
Loading

closes #210552

Before:

  • Top dependencies API returned baseline and comparison timeseries data.
Screenshot 2025-02-14 at 14 27 28

After:

  • Removing timeseries and comparison data.
  • The API is ~2s faster than before.
  • Response size is also smaller after removing the timeseries data.
Screenshot 2025-02-14 at 14 26 34

Created a new API: POST /internal/apm/dependencies/top_dependencies/statistics to fetch the statistics for the visible dependencies.

@cauemarcondes cauemarcondes added release_note:enhancement backport:skip This commit does not require backporting v9.1.0 labels Feb 17, 2025
@cauemarcondes cauemarcondes requested a review from a team as a code owner February 17, 2025 13:27
@botelastic botelastic bot added the Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team label Feb 17, 2025
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

@cauemarcondes cauemarcondes added backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) backport:prev-major Backport to (8.x, 8.18, 8.17, 8.16) the previous major branch and other branches in development and removed backport:skip This commit does not require backporting v9.1.0 labels Feb 17, 2025
crespocarlos
crespocarlos reviewed Feb 20, 2025
View reviewed changes
...gins/apm/public/components/app/dependencies_inventory/dependencies_inventory_table/index.tsx Outdated
@@ -27,6 +31,7 @@ export function DependenciesInventoryTable() {
} = useApmParams('/dependencies/inventory');
const { onPageReady } = usePerformanceContext();
const { start, end } = useTimeRange({ rangeFrom, rangeTo });
const [renderedItems, setRenderedItems] = useState<FormattedSpanMetricGroup[]>([]);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to reset renderedItems whenever a request is made to GET /internal/apm/dependencies/top_dependencies. This is because when clicking on refresh, the /statistics endpoint is called in parallel with /top_dependencies.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This happens on all pages, and that's because the use_fetcher depends on the timeRangeId, which is updated every time the refresh button is clicked, invalidating all functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, but looks like, in this case, we are making an unnecessary request to statistics aren't we? I.e: it's fetching data for dependencies that will be replaced once top_dependencies responds. And once it responds, statistics will be called again for the actual dependencies it needs to fetch data for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but looks like, in this case, we are making an unnecessary request to statistics aren't we?

Not really, the stats API is only called once on every refresh.
Screenshot 2025-02-24 at 14 09 44

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 . That's not what I'm seeing

top_dep.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😲

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested pointing to another cluster, same as yours, and the extra call happened here too. Let me take a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I made some changes, I mainly removed the renderedItems, we'll need to close look into it to make sure it works as it should. I also add a new option on the userFetcher to not invalidate the dependencies when the refresh button is clicked.

@miloszmarcinkowski
Copy link
Contributor

miloszmarcinkowski commented Feb 25, 2025 •
edited
Loading

The comparison checkbox stopped refreshing statistics, the refresh button must be clicked to see any changes.

Screen.Recording.2025-02-25.at.16.55.35.mov

miloszmarcinkowski
miloszmarcinkowski reviewed Feb 25, 2025
View reviewed changes
Copy link
Contributor

@miloszmarcinkowski miloszmarcinkowski left a comment

Choose a reason for hiding this comment

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

A lot of work done here 👏

I found one issue described in the earlier comment and one typo.

@cauemarcondes
Copy link
Contributor Author

The comparison checkbox stopped refreshing statistics, the refresh button must be clicked to see any changes.

I missed to put the comparison and offset back on the deps... thanks for that.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
apm 2.3MB 2.3MB +2.8KB

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 119 120 +1
Unknown metric groups

ESLint disabled line counts

id before after diff
apm 68 69 +1

Total ESLint disabled count

id before after diff
apm 81 82 +1

History

crespocarlos
crespocarlos reviewed Feb 26, 2025
View reviewed changes
...gins/apm/public/components/app/dependencies_inventory/dependencies_inventory_table/index.tsx
)
.slice(page * pageSize, (page + 1) * pageSize)
.map(({ name }) => name)
.sort()
Copy link
Contributor

@crespocarlos crespocarlos Feb 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need to sort the list for a second time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crespocarlos crespocarlos crespocarlos left review comments

@miloszmarcinkowski miloszmarcinkowski Awaiting requested review from miloszmarcinkowski

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
backport:prev-major Backport to (8.x, 8.18, 8.17, 8.16) the previous major branch and other branches in development backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) release_note:enhancement Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[APM] Breakdown Top dependencies API
4 participants
@cauemarcondes @elasticmachine @miloszmarcinkowski @crespocarlos