Skip to content

[Cloud Security] Adding telemetry collection condition based on render condition #208758

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

Merged
JordanSh merged 9 commits into from
Feb 11, 2025
Merged

[Cloud Security] Adding telemetry collection condition based on render condition #208758

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f27fd78
added telemetry collection condition based on render condition
JordanSh Jan 29, 2025
79f4ef8
merge
JordanSh Jan 30, 2025
e46bdd0
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine Jan 30, 2025
6ad1171
lint
JordanSh Jan 31, 2025
8927342
merge
JordanSh Jan 31, 2025
1749092
Merge branch 'main' of https://github.com/elastic/kibana into telemet…
JordanSh Feb 2, 2025
b972974
added version to naming so it will be easier to track after fix telem…
JordanSh Feb 2, 2025
b4f4a9a
merge
JordanSh Feb 11, 2025
12eb8de
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine Feb 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions x-pack/platform/packages/shared/kbn-cloud-security-posture/common/utils/ui_metrics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ import type { UsageCollectionSetup } from '@kbn/usage-collection-plugin/public';

export const APP_NAME = 'cloud-security';

export const MISCONFIGURATION_INSIGHT = 'misconfiguration-insight' as const;
export const VULNERABILITIES_INSIGHT = 'vulnerabilities-insight' as const;
export const MISCONFIGURATION_INSIGHT = 'misconfiguration-insight-v2' as const;
export const VULNERABILITIES_INSIGHT = 'vulnerabilities-insight-v2' as const;
Comment on lines +13 to +14
Copy link
Copy Markdown
Contributor Author

@JordanSh JordanSh Feb 2, 2025

Choose a reason for hiding this comment

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

As requested by @CohenIdo, added version to namings so it will be easier to track fixed version telemetry reports

export const MISCONFIGURATION_INSIGHT_HOST_DETAILS =
`${MISCONFIGURATION_INSIGHT}-host-details` as const;
export const MISCONFIGURATION_INSIGHT_USER_DETAILS =
Expand Down
16 changes: 8 additions & 8 deletions ...ty_solution/public/flyout/document_details/shared/components/misconfiguration_insight.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,19 +87,19 @@ export const MisconfigurationsInsight: React.FC<MisconfigurationsInsightProps> =
'newExpandableFlyoutNavigationEnabled'
);

useEffect(() => {
if (telemetryKey) {
uiMetricService.trackUiMetric(METRIC_TYPE.COUNT, telemetryKey);
}
}, [telemetryKey, renderingId]);

const passedFindings = data?.count.passed || 0;
const failedFindings = data?.count.failed || 0;
const totalFindings = useMemo(
() => passedFindings + failedFindings,
[passedFindings, failedFindings]
);
const hasMisconfigurationFindings = totalFindings > 0;
const shouldRender = totalFindings > 0; // this component only renders if there are findings

useEffect(() => {
Copy link
Copy Markdown
Contributor

@maxcold maxcold Jan 30, 2025

Choose a reason for hiding this comment

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

While it's a fix over the previous version where we tracked even when the particular insight wasn't even rendered, I wonder what do we want to track overall? With this change the counter will go up with every re-render of the component, for example when a user expands the left pane, switches between Overview, Table, JSON tabs and who knows when else as it's hard to reason about reliable component rerender in React. If our goal is to understand how many users had the component showing up at least once, I guess the current approach would do. But it's harder to make more complex conclusions from this telemetry. Probably not for this PR, but we have a preview link in the component from the counter. We could count clicks to track actual interactions with the component. Or if we want to see for how many alerts per user the data is present, we need to add an alert uuid to tracking to be able to then group by re-renders for the same alerts

Copy link
Copy Markdown
Contributor Author

@JordanSh JordanSh Jan 30, 2025

Choose a reason for hiding this comment

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

good point max. I believe this was solved by [Cloud Security] Collecting telemetry of graph visualization usage

Copy link
Copy Markdown
Contributor

@CohenIdo CohenIdo Feb 2, 2025

Choose a reason for hiding this comment

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

I’m curious why the condition is checked in MisconfigurationsInsight (and in VulnerabilitiesInsight) before calling them. In other words, why not call these functions only if there are findings?

Copy link
Copy Markdown
Contributor Author

@JordanSh JordanSh Feb 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

not sure what you mean, this is exactly whats happening. flow is:

Checking we have findings, which is also the render condition

  const shouldRender = totalFindings > 0; // this component only renders if there are findings

if shouldRender is true (meaning we have findings), report metric

if (shouldRender && telemetryKey) {
      uiMetricService.trackUiMetric(METRIC_TYPE.COUNT, telemetryKey);
    }

Meaning we only call the function if there are findings

Copy link
Copy Markdown
Contributor

@maxcold maxcold Feb 3, 2025

Choose a reason for hiding this comment

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

@JordanSh I checked and every switch between Overview/Table/JSON tabs still increases the counter. Again, not sure if it's a problem concerning this particular PR, I'm just wondering what we actually want to count with the current approach and if we are able to clean those meaningful counts on the reporting side

Copy link
Copy Markdown
Contributor

@christineweng christineweng Feb 5, 2025

Choose a reason for hiding this comment

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

@JordanSh I have a PR open to update the count badges: when user clicks on the count, it will go to the respective cloud insight table. If you do want to track clicks, you can do it in that call back.

Copy link
Copy Markdown
Contributor Author

@JordanSh JordanSh Feb 6, 2025

Choose a reason for hiding this comment

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

We can add that as well, i dont think it replaces this view count that we are trying to measure. Thanks for letting me know!
@CohenIdo do we want to add tracker for those counter clicks?

if (shouldRender && telemetryKey) {
Copy link
Copy Markdown
Contributor

@christineweng christineweng Feb 5, 2025

Choose a reason for hiding this comment

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

Have you considered typing telemetryKey it as required? Looks like it is always passed

Copy link
Copy Markdown
Contributor Author

@JordanSh JordanSh Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

yeah makes sense, can do that

ended up not doing that, it isn't required for tests, and in general only required for telemetry and not for the component it self in order to function. for those reasons i've decided to keep it optional for now.

uiMetricService.trackUiMetric(METRIC_TYPE.COUNT, telemetryKey);
}
}, [shouldRender, telemetryKey, renderingId]);

const misconfigurationsStats = useMemo(
() => getFindingsStats(passedFindings, failedFindings),
Expand Down Expand Up @@ -161,7 +161,7 @@ export const MisconfigurationsInsight: React.FC<MisconfigurationsInsightProps> =
]
);

if (!hasMisconfigurationFindings) return null;
if (!shouldRender) return null;

return (
<EuiFlexItem data-test-subj={dataTestSubj}>
Expand Down
17 changes: 9 additions & 8 deletions ...ity_solution/public/flyout/document_details/shared/components/vulnerabilities_insight.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,19 +84,14 @@ export const VulnerabilitiesInsight: React.FC<VulnerabilitiesInsightProps> = ({
'newExpandableFlyoutNavigationEnabled'
);

useEffect(() => {
if (telemetryKey) {
uiMetricService.trackUiMetric(METRIC_TYPE.COUNT, telemetryKey);
}
}, [telemetryKey, renderingId]);

const { CRITICAL = 0, HIGH = 0, MEDIUM = 0, LOW = 0, NONE = 0 } = data?.count || {};
const totalVulnerabilities = useMemo(
() => CRITICAL + HIGH + MEDIUM + LOW + NONE,
[CRITICAL, HIGH, MEDIUM, LOW, NONE]
);

const hasVulnerabilitiesFindings = useMemo(
// this component only renders if there are findings
const shouldRender = useMemo(
() =>
hasVulnerabilitiesData({
critical: CRITICAL,
Expand All @@ -108,6 +103,12 @@ export const VulnerabilitiesInsight: React.FC<VulnerabilitiesInsightProps> = ({
[CRITICAL, HIGH, MEDIUM, LOW, NONE]
);

useEffect(() => {
if (shouldRender && telemetryKey) {
uiMetricService.trackUiMetric(METRIC_TYPE.COUNT, telemetryKey);
}
}, [shouldRender, telemetryKey, renderingId]);

const vulnerabilitiesStats = useMemo(
() =>
getVulnerabilityStats(
Expand Down Expand Up @@ -177,7 +178,7 @@ export const VulnerabilitiesInsight: React.FC<VulnerabilitiesInsightProps> = ({
]
);

if (!hasVulnerabilitiesFindings) return null;
if (!shouldRender) return null;

return (
<EuiFlexItem data-test-subj={dataTestSubj}>
Expand Down