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 Author

@code-asher code-asher Dec 14, 2025

Choose a reason for hiding this comment

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

Yeah I 100% agree that is way better but that will also require a refactor of stats right, since those deployment stats are derived from the agent stats. We would have to anonymize the stats, or have some other way to aggregate them without storing the individual stats.

We did consider anonymization but decided for now we would go with disabling (for context the client this is for is unable to store personal employee data which apparently includes these stats).

Possibly we could rename this flag to something that more fully encapsulates both. --usage-stats-disable maybe?? --stats-disable? Separate --stats-app-enable and --stats-agent-enable maybe? Going to merge for now but I will follow up with another PR if we have a good name (I will bring it up in the mango standup too and see if there are any ideas).

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.

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 be misunderstanding but this seems to be correctly generated, when I use --help it wraps in the same way.

Copy link
Member

Choose a reason for hiding this comment

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

This wasn't about formatting, just me suggesting alternative wording.

}

// 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 [...]

Copy link
Member Author

@code-asher code-asher Dec 14, 2025

Choose a reason for hiding this comment

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

Honestly I could be a bit wrong, but there is a variable called "metrics" which includes things like cpu, memory, etc and those are still emitted (we do not store or display them with template insights or agent stats as far as I could tell).

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I wanted to minimize the diff but this felt janky to me too. I am a bit late on this so going to merge but I will follow up with another PR.

@code-asher code-asher force-pushed the asher/disable-template-insights branch from 29eb053 to 54e725e Compare December 14, 2025 02:45
@code-asher code-asher enabled auto-merge (squash) December 14, 2025 02:49
@code-asher code-asher merged commit 27f0413 into main Dec 14, 2025
53 of 56 checks passed
@code-asher code-asher deleted the asher/disable-template-insights branch December 14, 2025 03:00
@github-actions github-actions bot locked and limited conversation to collaborators Dec 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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