Skip to content

Conversation

@code-asher
Copy link
Member

@code-asher code-asher commented Nov 26, 2025

Closes #20399

I think half of the added lines are just moving the big mock template insights object in the related story.

To summarize the commit messages:

  • Do not log stats to the database.
  • Return errors on the insight endpoints.
  • Update the frontend to show those errors.
  • Also fixes an issue with getting the user status count via codersdk, since I added a test to ensure it was not disabled by this flag and it was sending the wrong payload.

@code-asher code-asher force-pushed the asher/disable-template-insights branch 6 times, most recently from 46fd3d8 to f5abd4e Compare November 26, 2025 04:52
@code-asher code-asher requested a review from mafredri December 2, 2025 21:57
@bpmct bpmct added the doc-check Assign this label to PRs to check for any doc changes. label Dec 4, 2025
@github-actions
Copy link

github-actions bot commented Dec 4, 2025

Copy link
Member

bpmct commented Dec 4, 2025

📚 Documentation Check

✅ Updates Needed

None - API documentation already updated in this PR

📝 New Docs Needed

  • docs/admin/monitoring/insights.md - Create a new guide explaining template insights feature, what data is collected, how to view insights in the UI, and how to disable it using --disable-template-insights flag. This would help administrators understand the feature and its privacy/performance implications.

📋 Already Documented

The PR appropriately updated:

📖 Recommendation

While the reference documentation is complete, consider adding a user-facing guide that explains:

  1. What template insights are and what they show
  2. What data is collected and stored
  3. Performance/storage impact of insights
  4. When and why administrators might want to disable them
  5. How disabling affects the UI (what happens to the insights pages)

This would fit well in the docs/admin/monitoring/ section alongside other observability features.


This comment was generated by an AI Agent through Coder Tasks

@bpmct
Copy link
Member

bpmct commented Dec 4, 2025

Ha - that's a lot. Feel free to disregard

bpmct added a commit that referenced this pull request Dec 7, 2025
Proactively documents the --disable-template-insights flag from PR #20940
before it merges, ensuring documentation is ready when the feature lands.
This effectively disables template insights, as there will be no data.
Prometheus remains unaffected; this only prevents our own storage of the
stats.
It takes a timezone offset, not a date.
Now errors show up in the middle of the panel.  This will include if the
insights are disabled.
@code-asher code-asher force-pushed the asher/disable-template-insights branch from f5abd4e to 092cca4 Compare December 10, 2025 01:51
@code-asher code-asher removed the request for review from mafredri December 10, 2025 01:51
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

All-in-all looks good, but I have one concern regarding agent stats (see comments). The rest are smaller suggestions/nits.

Whilst I think the FE changes look good, I'll defer approval to someone from the FE team as I believe they may want emotion to be converted to tailwind whilst you're in here.

SessionCountSsh: stat.sessionCountSSH,
}, false)
createdAt = createdAt.Add(30 * time.Second)
}
Copy link
Member

Choose a reason for hiding this comment

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

Consider if this needs a rethink in context of the previous comment about disabling agent stats batching.

Copy link
Member Author

Choose a reason for hiding this comment

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

I might not quite be following, normally the reporter skips adding stats to the batcher but in the test here we are manually adding batches instead of delegating to the reporter, so I had to skip those separately here when we test disabled stats (otherwise we would have stats even though they are disabled).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this change makes sense. My "consider" was tied to the other concern about not storing agent stats, and depending on the direction taken there (two options, 1, restore agent stats are always stored, or 2, keep but document the behavior).

This callout was for option 1.

Group: &deploymentGroupIntrospection,
YAML: "disableTemplateInsights",
Name: "Enable Template Insights",
Description: "Enable the collection and display of template insights along with the associated API endpoints. This will also enable aggregating these insights into daily active users, application usage, and transmission rates for overall deployment stats. When disabled, these values will be zero, which will also affect what the bottom deployment overview bar displays. Disabling will also prevent Prometheus collection of these values.",
Copy link
Member Author

@code-asher code-asher Dec 11, 2025

Choose a reason for hiding this comment

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

I think this is everything affected by disabling collection of these stats. I am not sure I worded it clearly though, I went through so many revisions 😅

Copy link
Member

Choose a reason for hiding this comment

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

It's on the verbose side, but I think it captures everything. Nice work. Just to be annoying though, I will propose the idea of separating template insights and agent stats, i.e. having two options. I think that would be clearer since the naming is lossy. The name "enable template insights" affects deployment stats but it's not captured outside the description.

Food for thought, not a requirement.

Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

One suggestion, one question, otherwise 👍🏻 for BE!

Comment on lines +278 to +279
be zero, which will also affect what the bottom deployment overview
bar displays. Disabling will also prevent Prometheus collection of
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
be zero, which will also affect what the bottom deployment overview
bar displays. Disabling will also prevent Prometheus collection of
be zero. Disabling this will also disable traffic and connection insights in the deployment stats shown to admins in the bottom bar of the Coder UI.
Disabling will also prevent Prometheus collection of

Ignore indentation.

}

// update prometheus metrics
// update prometheus metrics (even if template insights are disabled)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, is this conflicting with the help text added to the deployment flag?

Disabling will also prevent Prometheus collection of [...]

Comment on lines +259 to +260
data={templateInsights?.data?.interval_reports}
error={templateInsights?.error}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
data={templateInsights?.data?.interval_reports}
error={templateInsights?.error}
data={templateInsights.data?.interval_reports}
error={templateInsights.error}

I think a lot of these question marks are no longer needed given the changes to the types

Comment on lines +268 to +269
data={templateInsights?.data?.report?.apps_usage}
error={templateInsights?.error}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
data={templateInsights?.data?.report?.apps_usage}
error={templateInsights?.error}
data={templateInsights.data?.report?.apps_usage}
error={templateInsights.error}

Comment on lines +277 to +278
data={templateInsights?.data?.report?.parameters_usage}
error={templateInsights?.error}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
data={templateInsights?.data?.report?.parameters_usage}
error={templateInsights?.error}
data={templateInsights.data?.report?.parameters_usage}
error={templateInsights.error}

Comment on lines +423 to +424
{!error && !users && <Loader css={{ height: "100%" }} />}
{(error || users?.length === 0) && <NoDataAvailable error={error} />}
Copy link
Member

Choose a reason for hiding this comment

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

I kinda wish we were showing the errors/loading states with early returns or ternarys or something so that these weren't all just independent conditions where we cross our fingers that they're all properly in sync

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

Labels

doc-check Assign this label to PRs to check for any doc changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add deployment flag to disable/anonymize template insights data collection

5 participants