Skip to content

chore: remove notifications experiment #14869

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

Merged
merged 8 commits into from
Oct 1, 2024
1 change: 0 additions & 1 deletion cli/notifications_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ func createOpts(t *testing.T) *coderdtest.Options {
t.Helper()

dt := coderdtest.DeploymentValues(t)
dt.Experiments = []string{string(codersdk.ExperimentNotifications)}
return &coderdtest.Options{
DeploymentValues: dt,
}
Expand Down
94 changes: 42 additions & 52 deletions cli/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,16 @@ import (

"cdr.dev/slog"
"cdr.dev/slog/sloggers/sloghuman"
"github.com/coder/coder/v2/coderd/entitlements"
"github.com/coder/coder/v2/coderd/notifications/reports"
"github.com/coder/coder/v2/coderd/runtimeconfig"
"github.com/coder/pretty"
"github.com/coder/quartz"
"github.com/coder/retry"
"github.com/coder/serpent"
"github.com/coder/wgtunnel/tunnelsdk"

"github.com/coder/coder/v2/coderd/entitlements"
"github.com/coder/coder/v2/coderd/notifications/reports"
"github.com/coder/coder/v2/coderd/runtimeconfig"

"github.com/coder/coder/v2/buildinfo"
"github.com/coder/coder/v2/cli/clilog"
"github.com/coder/coder/v2/cli/cliui"
Expand Down Expand Up @@ -679,10 +680,6 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
options.OIDCConfig = oc
}

experiments := coderd.ReadExperiments(
options.Logger, options.DeploymentValues.Experiments.Value(),
)

// We'll read from this channel in the select below that tracks shutdown. If it remains
// nil, that case of the select will just never fire, but it's important not to have a
// "bare" read on this channel.
Expand Down Expand Up @@ -946,6 +943,33 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
return xerrors.Errorf("write config url: %w", err)
}

// Manage notifications.
cfg := options.DeploymentValues.Notifications
metrics := notifications.NewMetrics(options.PrometheusRegistry)
helpers := templateHelpers(options)

// The enqueuer is responsible for enqueueing notifications to the given store.
enqueuer, err := notifications.NewStoreEnqueuer(cfg, options.Database, helpers, logger.Named("notifications.enqueuer"), quartz.NewReal())
if err != nil {
return xerrors.Errorf("failed to instantiate notification store enqueuer: %w", err)
}
options.NotificationsEnqueuer = enqueuer

// The notification manager is responsible for:
// - creating notifiers and managing their lifecycles (notifiers are responsible for dequeueing/sending notifications)
// - keeping the store updated with status updates
notificationsManager, err := notifications.NewManager(cfg, options.Database, helpers, metrics, logger.Named("notifications.manager"))
if err != nil {
return xerrors.Errorf("failed to instantiate notification manager: %w", err)
}

// nolint:gocritic // TODO: create own role.
notificationsManager.Run(dbauthz.AsSystemRestricted(ctx))

// Run report generator to distribute periodic reports.
notificationReportGenerator := reports.NewReportGenerator(ctx, logger.Named("notifications.report_generator"), options.Database, options.NotificationsEnqueuer, quartz.NewReal())
defer notificationReportGenerator.Close()

// Since errCh only has one buffered slot, all routines
// sending on it must be wrapped in a select/default to
// avoid leaving dangling goroutines waiting for the
Expand Down Expand Up @@ -1002,38 +1026,6 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
options.WorkspaceUsageTracker = tracker
defer tracker.Close()

// Manage notifications.
var (
notificationsManager *notifications.Manager
)
if experiments.Enabled(codersdk.ExperimentNotifications) {
cfg := options.DeploymentValues.Notifications
metrics := notifications.NewMetrics(options.PrometheusRegistry)
helpers := templateHelpers(options)

// The enqueuer is responsible for enqueueing notifications to the given store.
enqueuer, err := notifications.NewStoreEnqueuer(cfg, options.Database, helpers, logger.Named("notifications.enqueuer"), quartz.NewReal())
if err != nil {
return xerrors.Errorf("failed to instantiate notification store enqueuer: %w", err)
}
options.NotificationsEnqueuer = enqueuer

// The notification manager is responsible for:
// - creating notifiers and managing their lifecycles (notifiers are responsible for dequeueing/sending notifications)
// - keeping the store updated with status updates
notificationsManager, err = notifications.NewManager(cfg, options.Database, helpers, metrics, logger.Named("notifications.manager"))
if err != nil {
return xerrors.Errorf("failed to instantiate notification manager: %w", err)
}

// nolint:gocritic // TODO: create own role.
notificationsManager.Run(dbauthz.AsSystemRestricted(ctx))

// Run report generator to distribute periodic reports.
notificationReportGenerator := reports.NewReportGenerator(ctx, logger.Named("notifications.report_generator"), options.Database, options.NotificationsEnqueuer, quartz.NewReal())
defer notificationReportGenerator.Close()
}

// Wrap the server in middleware that redirects to the access URL if
// the request is not to a local IP.
var handler http.Handler = coderAPI.RootHandler
Expand Down Expand Up @@ -1153,19 +1145,17 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
// Cancel any remaining in-flight requests.
shutdownConns()

if notificationsManager != nil {
// Stop the notification manager, which will cause any buffered updates to the store to be flushed.
// If the Stop() call times out, messages that were sent but not reflected as such in the store will have
// their leases expire after a period of time and will be re-queued for sending.
// See CODER_NOTIFICATIONS_LEASE_PERIOD.
cliui.Info(inv.Stdout, "Shutting down notifications manager..."+"\n")
err = shutdownWithTimeout(notificationsManager.Stop, 5*time.Second)
if err != nil {
cliui.Warnf(inv.Stderr, "Notifications manager shutdown took longer than 5s, "+
"this may result in duplicate notifications being sent: %s\n", err)
} else {
cliui.Info(inv.Stdout, "Gracefully shut down notifications manager\n")
}
// Stop the notification manager, which will cause any buffered updates to the store to be flushed.
// If the Stop() call times out, messages that were sent but not reflected as such in the store will have
// their leases expire after a period of time and will be re-queued for sending.
// See CODER_NOTIFICATIONS_LEASE_PERIOD.
cliui.Info(inv.Stdout, "Shutting down notifications manager..."+"\n")
err = shutdownWithTimeout(notificationsManager.Stop, 5*time.Second)
if err != nil {
cliui.Warnf(inv.Stderr, "Notifications manager shutdown took longer than 5s, "+
"this may result in duplicate notifications being sent: %s\n", err)
} else {
cliui.Info(inv.Stdout, "Gracefully shut down notifications manager\n")
}

// Shut down provisioners before waiting for WebSockets
Expand Down
10 changes: 4 additions & 6 deletions coderd/coderd.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,12 @@ import (
"tailscale.com/util/singleflight"

"cdr.dev/slog"
"github.com/coder/quartz"
"github.com/coder/serpent"

"github.com/coder/coder/v2/coderd/entitlements"
"github.com/coder/coder/v2/coderd/idpsync"
"github.com/coder/coder/v2/coderd/runtimeconfig"
"github.com/coder/quartz"
"github.com/coder/serpent"

agentproto "github.com/coder/coder/v2/agent/proto"
"github.com/coder/coder/v2/buildinfo"
Expand Down Expand Up @@ -1257,10 +1258,7 @@ func New(options *Options) *API {
})
})
r.Route("/notifications", func(r chi.Router) {
r.Use(
apiKeyMiddleware,
httpmw.RequireExperiment(api.Experiments, codersdk.ExperimentNotifications),
)
r.Use(apiKeyMiddleware)
r.Get("/settings", api.notificationsSettings)
r.Put("/settings", api.putNotificationsSettings)
r.Route("/templates", func(r chi.Router) {
Expand Down
9 changes: 7 additions & 2 deletions coderd/notifications/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ type Manager struct {

runOnce sync.Once
stopOnce sync.Once
doneOnce sync.Once
stop chan any
done chan any

Expand Down Expand Up @@ -153,7 +154,9 @@ func (m *Manager) Run(ctx context.Context) {
// events, creating a notifier, and publishing bulk dispatch result updates to the store.
func (m *Manager) loop(ctx context.Context) error {
defer func() {
close(m.done)
m.doneOnce.Do(func() {
close(m.done)
})
m.log.Info(context.Background(), "notification manager stopped")
}()

Expand Down Expand Up @@ -364,7 +367,9 @@ func (m *Manager) Stop(ctx context.Context) error {
// If the notifier hasn't been started, we don't need to wait for anything.
// This is only really during testing when we want to enqueue messages only but not deliver them.
if m.notifier == nil {
close(m.done)
m.doneOnce.Do(func() {
close(m.done)
})
} else {
m.notifier.stop()
}
Expand Down
1 change: 0 additions & 1 deletion coderd/notifications/notifications_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1187,7 +1187,6 @@ func createOpts(t *testing.T) *coderdtest.Options {
t.Helper()

dt := coderdtest.DeploymentValues(t)
dt.Experiments = []string{string(codersdk.ExperimentNotifications)}
return &coderdtest.Options{
DeploymentValues: dt,
}
Expand Down
2 changes: 1 addition & 1 deletion coderd/notifications/reports/generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func NewReportGenerator(ctx context.Context, logger slog.Logger, db database.Sto
return nil
}

err = reportFailedWorkspaceBuilds(ctx, logger, db, enqueuer, clk)
err = reportFailedWorkspaceBuilds(ctx, logger, tx, enqueuer, clk)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing the main db object through caused a deadlock; we need to pass the transaction-scoped db object instead.

if err != nil {
return xerrors.Errorf("unable to generate reports with failed workspace builds: %w", err)
}
Expand Down
1 change: 0 additions & 1 deletion coderd/notifications_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ func createOpts(t *testing.T) *coderdtest.Options {
t.Helper()

dt := coderdtest.DeploymentValues(t)
dt.Experiments = []string{string(codersdk.ExperimentNotifications)}
return &coderdtest.Options{
DeploymentValues: dt,
}
Expand Down
2 changes: 1 addition & 1 deletion codersdk/deployment.go
Original file line number Diff line number Diff line change
Expand Up @@ -2901,7 +2901,7 @@ const (
// users to opt-in to via --experimental='*'.
// Experiments that are not ready for consumption by all users should
// not be included here and will be essentially hidden.
var ExperimentsAll = Experiments{ExperimentNotifications}
var ExperimentsAll = Experiments{}

// Experiments is a list of experiments.
// Multiple experiments may be enabled at the same time.
Expand Down
1 change: 0 additions & 1 deletion enterprise/coderd/coderd.go
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,6 @@ func New(ctx context.Context, options *Options) (_ *API, err error) {
// with the below route, we need to register this route without any mounts or groups to make both work.
r.With(
apiKeyMiddleware,
httpmw.RequireExperiment(api.AGPL.Experiments, codersdk.ExperimentNotifications),
httpmw.ExtractNotificationTemplateParam(options.Database),
).Put("/notifications/templates/{notification_template}/method", api.updateNotificationTemplateMethod)
})
Expand Down
1 change: 0 additions & 1 deletion enterprise/coderd/notifications_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ func createOpts(t *testing.T) *coderdenttest.Options {
t.Helper()

dt := coderdtest.DeploymentValues(t)
dt.Experiments = []string{string(codersdk.ExperimentNotifications)}
return &coderdenttest.Options{
Options: &coderdtest.Options{
DeploymentValues: dt,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { docs } from "utils/docs";
* All types of feature that we are currently supporting. Defined as record to
* ensure that we can't accidentally make typos when writing the badge text.
*/
const featureStageBadgeTypes = {
export const featureStageBadgeTypes = {
beta: "beta",
experimental: "experimental",
} as const satisfies Record<string, ReactNode>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export const NotificationsPage: FC = () => {
title="Notifications"
description="Control delivery methods for notifications on this deployment."
layout="fluid"
featureStage={"beta"}
>
<Tabs active={tab}>
<TabsList>
Expand Down
9 changes: 4 additions & 5 deletions site/src/pages/DeploySettingsPage/Sidebar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import NotificationsIcon from "@mui/icons-material/NotificationsNoneOutlined";
import Globe from "@mui/icons-material/PublicOutlined";
import ApprovalIcon from "@mui/icons-material/VerifiedUserOutlined";
import VpnKeyOutlined from "@mui/icons-material/VpnKeyOutlined";
import { FeatureStageBadge } from "components/FeatureStageBadge/FeatureStageBadge";
import { GitIcon } from "components/Icons/GitIcon";
import {
Sidebar as BaseSidebar,
Expand Down Expand Up @@ -51,11 +52,9 @@ export const Sidebar: FC = () => {
<SidebarNavItem href="observability" icon={InsertChartIcon}>
Observability
</SidebarNavItem>
{experiments.includes("notifications") && (
<SidebarNavItem href="notifications" icon={NotificationsIcon}>
Notifications
</SidebarNavItem>
)}
<SidebarNavItem href="notifications" icon={NotificationsIcon}>
Notifications <FeatureStageBadge contentType="beta" size="sm" />
</SidebarNavItem>
</BaseSidebar>
);
};
5 changes: 3 additions & 2 deletions site/src/pages/ManagementSettingsPage/SidebarView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -148,11 +148,12 @@ const DeploymentSettingsNavigation: FC<DeploymentSettingsNavigationProps> = ({
Users
</SidebarNavSubItem>
)}
{experiments.includes("notifications") && (
<Stack direction="row" alignItems="center" css={{ gap: 0 }}>
<SidebarNavSubItem href="notifications">
Notifications
</SidebarNavSubItem>
)}
<FeatureStageBadge contentType="beta" size="sm" />
</Stack>
</Stack>
)}
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ export const NotificationsPage: FC = () => {
title="Notifications"
description="Configure your notification preferences. Icons on the right of each notification indicate delivery method, either SMTP or Webhook."
layout="fluid"
featureStage="beta"
>
{ready ? (
<Stack spacing={4}>
Expand Down
36 changes: 26 additions & 10 deletions site/src/pages/UserSettingsPage/Section.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import type { Interpolation, Theme } from "@emotion/react";
import {
FeatureStageBadge,
type featureStageBadgeTypes,
} from "components/FeatureStageBadge/FeatureStageBadge";
import { Stack } from "components/Stack/Stack";
import type { FC, ReactNode } from "react";

type SectionLayout = "fixed" | "fluid";
Expand All @@ -13,6 +18,7 @@ export interface SectionProps {
layout?: SectionLayout;
className?: string;
children?: ReactNode;
featureStage?: keyof typeof featureStageBadgeTypes;
}

export const Section: FC<SectionProps> = ({
Expand All @@ -24,6 +30,7 @@ export const Section: FC<SectionProps> = ({
className = "",
children,
layout = "fixed",
featureStage,
}) => {
return (
<section className={className} id={id} data-testid={id}>
Expand All @@ -32,16 +39,25 @@ export const Section: FC<SectionProps> = ({
<div css={styles.header}>
<div>
{title && (
<h4
css={{
fontSize: 24,
fontWeight: 500,
margin: 0,
marginBottom: 8,
}}
>
{title}
</h4>
<Stack direction={"row"} alignItems="center">
<h4
css={{
fontSize: 24,
fontWeight: 500,
margin: 0,
marginBottom: 8,
}}
>
{title}
</h4>
{featureStage && (
<FeatureStageBadge
contentType={featureStage}
size="lg"
css={{ marginBottom: "5px" }}
/>
)}
</Stack>
)}
{description && typeof description === "string" && (
<p css={styles.description}>{description}</p>
Expand Down
9 changes: 4 additions & 5 deletions site/src/pages/UserSettingsPage/Sidebar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import NotificationsIcon from "@mui/icons-material/NotificationsNoneOutlined";
import AccountIcon from "@mui/icons-material/Person";
import VpnKeyOutlined from "@mui/icons-material/VpnKeyOutlined";
import type { User } from "api/typesGenerated";
import { FeatureStageBadge } from "components/FeatureStageBadge/FeatureStageBadge";
import { GitIcon } from "components/Icons/GitIcon";
import {
Sidebar as BaseSidebar,
Expand Down Expand Up @@ -57,11 +58,9 @@ export const Sidebar: FC<SidebarProps> = ({ user }) => {
<SidebarNavItem href="tokens" icon={VpnKeyOutlined}>
Tokens
</SidebarNavItem>
{experiments.includes("notifications") && (
<SidebarNavItem href="notifications" icon={NotificationsIcon}>
Notifications
</SidebarNavItem>
)}
<SidebarNavItem href="notifications" icon={NotificationsIcon}>
Notifications <FeatureStageBadge contentType="beta" size="sm" />
</SidebarNavItem>
</BaseSidebar>
);
};
Loading