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

Improve telemetry with deeper insights into system usage #22337

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

Conversation

licitdev
Copy link
Member

@licitdev licitdev commented Apr 29, 2024

Scope

What's changed:

  • Filter by active users instead of all users
  • Filter by enabled extensions instead of all extensions
  • Add field counts: fields_max & fields_total
  • Add database size

Potential Risks / Drawbacks

  • More queries to the DB.

Review Notes / Questions

  • The queries had been tested on the DB containers, but wrapped in a try-catch in case it fails.

Fixes #21982

Copy link

changeset-bot bot commented Apr 29, 2024

🦋 Changeset detected

Latest commit: b4645f7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@directus/api Patch
directus Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@licitdev licitdev changed the title Improve telemetry Improve telemetry with deeper insights into system usage Apr 29, 2024
@licitdev licitdev marked this pull request as ready for review April 29, 2024 19:19
Co-authored-by: Daniel Biegler <[email protected]>
Copy link
Member

@paescuj paescuj left a comment

Choose a reason for hiding this comment

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

Great work ✨

  • For the newly introduced data (db size, and especially fields count) do we expect them to perform well? Wondering after seeing this comment:
    // Counts can be a little heavy if the table is very large, so we'll only ever execute 3 of these
    // queries simultaneously to not overload the database
  • Just because it might be good to know (and maybe add inline docs): Do (some of) the DB size queries require some special DB permissions?

* @returns Size of the database in bytes
*/
async getDatabaseSize(): Promise<number> {
return 0;
Copy link
Member

@paescuj paescuj May 9, 2024

Choose a reason for hiding this comment

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

Should we return null here as well as for failing getDatabaseSize calls in the dialects implementation so we can identify / distinguish these cases?

@paescuj paescuj self-assigned this May 9, 2024
'directus_roles',
'directus_shares',
{ collection: 'directus_dashboards' },
{ collection: 'directus_extensions', where: ['enabled', '=', true] },
Copy link
Contributor

Choose a reason for hiding this comment

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

Its not because of this PR but I noticed that we are actually counting bundles slightly wrong. Because a bundle is just a row and the inside-extensions point to it via its id a bundle with one extension inside counts as 2 and a bundle with two extensions inside counts as 3 - etc.

I looked up the original PR for the get-item-count.ts file, here, which already introduced this behavior but I dont see it being discussed there. Did we decide that we are OK with this somewhere else? (cc @rijkvanzanten maybe you know?)

Now that we added a possibility for a where clause we might be able to filter those out! I personally would be in favor of doing so. 😄 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe theres an easier way but off the top of my head I came up with this:

SELECT COUNT(id)
FROM directus_extensions
WHERE id NOT IN (
  SELECT DISTINCT x1.id 
  FROM directus_extensions as x1
  INNER JOIN directus_extensions as x2 ON x1.id = x2.bundle
);

Copy link
Member

Choose a reason for hiding this comment

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

A bundle should be counted as the sum of it's child extensions, excluding the parent 👍🏻

@paescuj paescuj assigned DanielBiegler and paescuj and unassigned paescuj May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

Improve Telemetry Reporting
4 participants