diff --git a/litellm/proxy/proxy_server.py b/litellm/proxy/proxy_server.py index ed2a5b89a32..9e8bb2fa93b 100644 --- a/litellm/proxy/proxy_server.py +++ b/litellm/proxy/proxy_server.py @@ -311,7 +311,6 @@ from litellm.constants import ( from litellm.exceptions import RejectedRequestError from litellm.integrations.custom_guardrail import CustomGuardrail, ModifyResponseException from litellm.integrations.custom_logger import CustomLogger -from litellm.integrations.SlackAlerting.slack_alerting import SlackAlerting from litellm.litellm_core_utils.agentic_loop_settings import ( validated_max_agentic_loops, ) @@ -16729,6 +16728,58 @@ def _nested_setting_source( return "db" if field_name in db_values else unset_source +def _alerting_field_value( + source: FieldSource, + field_name: str, + config_values: Mapping[str, JsonValue], + db_values: Mapping[str, JsonValue], +) -> JsonValue | None: + if source == "config": + return config_values.get(field_name) + if source == "db": + return db_values.get(field_name) + return None + + +def _alerting_stored_in_db(source: FieldSource) -> bool | None: + if source == "db": + return True + if source == "config": + return False + return None + + +def _alerting_field_response( + *, + settings: SettingsStore, + db_values: Mapping[str, JsonValue], + config_values: Mapping[str, JsonValue], + allowed_args: Mapping[str, str], + field_name: str, + field_info: FieldInfo, +) -> ConfigList: + field_default: Final[JsonValue] = _get_field_default(field_info) + field_source: Final[FieldSource] = _nested_setting_source( + settings, + db_values, + "alerting_args", + field_name, + field_default, + ) + field_value: Final = _alerting_field_value(field_source, field_name, config_values, db_values) + stored_in_db: Final = _alerting_stored_in_db(field_source) + return ConfigList( + field_name=field_name, + field_type=allowed_args[field_name], + field_description=field_info.description or "", + field_value=field_value, + stored_in_db=stored_in_db, + source=field_source, + field_default_value=field_default, + premium_field=field_name == "region_outage_alert_ttl", + ) + + @router.get( "/alerting/settings", description="Return the configurable alerting param, description, and current value", @@ -16739,7 +16790,7 @@ def _nested_setting_source( async def alerting_settings( user_api_key_dict: UserAPIKeyAuth = Depends(user_api_key_auth), ): - global proxy_logging_obj, prisma_client + global prisma_client """ Used by UI to generate 'alerting settings' page { @@ -16801,8 +16852,10 @@ async def alerting_settings( } ) - _slack_alerting: Final[SlackAlerting] = proxy_logging_obj.slack_alerting_instance - _slack_alerting_args_dict: Final = _slack_alerting.alerting_args.model_dump() + config_alerting_args_value: Final = settings.config_value("alerting_args") + config_alerting_args: Final[Mapping[str, JsonValue]] = ( + config_alerting_args_value if isinstance(config_alerting_args_value, Mapping) else MappingProxyType({}) + ) return_val: Final = [] @@ -16826,30 +16879,16 @@ async def alerting_settings( for field_name, field_info in SlackAlertingArgs.model_fields.items(): if field_name in allowed_args: - field_default: JsonValue = _get_field_default(field_info) - _stored_in_db: bool | None = None - if field_name in alerting_args_dict: - _stored_in_db = True - else: - _stored_in_db = False - - _response_obj = ConfigList( - field_name=field_name, - field_type=allowed_args[field_name], - field_description=field_info.description or "", - field_value=_slack_alerting_args_dict.get(field_name, field_default), - stored_in_db=_stored_in_db, - source=_nested_setting_source( - settings, - alerting_args_dict, - "alerting_args", - field_name, - field_default, - ), - field_default_value=field_default, - premium_field=(True if field_name == "region_outage_alert_ttl" else False), + return_val.append( + _alerting_field_response( + settings=settings, + db_values=alerting_args_dict, + config_values=config_alerting_args, + allowed_args=allowed_args, + field_name=field_name, + field_info=field_info, + ) ) - return_val.append(_response_obj) return return_val diff --git a/tests/unit/proxy/proxy_server/test_routes_model_metrics.py b/tests/unit/proxy/proxy_server/test_routes_model_metrics.py index b5536b7618c..3ea4fce8963 100644 --- a/tests/unit/proxy/proxy_server/test_routes_model_metrics.py +++ b/tests/unit/proxy/proxy_server/test_routes_model_metrics.py @@ -402,3 +402,109 @@ def test_alerting_settings_happy(client, auth_as, monkeypatch): "first_field_value": False, "first_field_type": "Boolean", } + +def test_alerting_settings_only_returns_explicitly_configured_values(client, auth_as, monkeypatch): + pc = MagicMock() + pc.db.litellm_config.find_first = AsyncMock(return_value=None) + monkeypatch.setattr(proxy_server, "prisma_client", pc) + + logging_obj = MagicMock() + args_model = MagicMock() + args_model.model_dump = MagicMock(return_value={"budget_alert_ttl": -30}) + logging_obj.slack_alerting_instance.alerting_args = args_model + monkeypatch.setattr(proxy_server, "proxy_logging_obj", logging_obj) + settings = SettingsStore("general_settings") + settings.load_yaml({}) + monkeypatch.setattr(proxy_server.proxy_config, "settings", settings) + + with auth_as(LitellmUserRoles.PROXY_ADMIN): + response = client.get("/alerting/settings") + + assert response.status_code == 200 + setting = next(item for item in response.json() if item["field_name"] == "budget_alert_ttl") + assert setting["field_value"] is None + assert setting["stored_in_db"] is None + + +def test_alerting_settings_returns_configured_values(client, auth_as, monkeypatch): + pc = MagicMock() + pc.db.litellm_config.find_first = AsyncMock(return_value=None) + monkeypatch.setattr(proxy_server, "prisma_client", pc) + settings = SettingsStore("general_settings") + settings.load_yaml({"alerting_args": {"budget_alert_ttl": 60}}) + monkeypatch.setattr(proxy_server.proxy_config, "settings", settings) + + with auth_as(LitellmUserRoles.PROXY_ADMIN): + response = client.get("/alerting/settings") + + assert response.status_code == 200 + setting = next(item for item in response.json() if item["field_name"] == "budget_alert_ttl") + assert setting["field_value"] == 60 + assert setting["stored_in_db"] is False + + +def test_alerting_settings_returns_database_values(client, auth_as, monkeypatch): + db_settings = MagicMock() + db_settings.param_value = {"alerting_args": {"budget_alert_ttl": 60}} + pc = MagicMock() + pc.db.litellm_config.find_first = AsyncMock(return_value=db_settings) + monkeypatch.setattr(proxy_server, "prisma_client", pc) + settings = SettingsStore("general_settings") + settings.load_yaml({}) + monkeypatch.setattr(proxy_server.proxy_config, "settings", settings) + + with auth_as(LitellmUserRoles.PROXY_ADMIN): + response = client.get("/alerting/settings") + + assert response.status_code == 200 + setting = next(item for item in response.json() if item["field_name"] == "budget_alert_ttl") + assert setting["field_value"] == 60 + assert setting["stored_in_db"] is True + +@pytest.mark.parametrize( + ("source", "expected_value", "expected_stored_in_db"), + [ + ("config", 60, False), + ("db", 90, True), + ("unset", None, None), + ("default", None, None), + ], +) +def test_alerting_field_helpers_cover_each_source( + source, expected_value, expected_stored_in_db +): + config_values = {"budget_alert_ttl": 60} + db_values = {"budget_alert_ttl": 90} + + assert ( + proxy_server._alerting_field_value( + source, + "budget_alert_ttl", + config_values, + db_values, + ) + == expected_value + ) + assert proxy_server._alerting_stored_in_db(source) is expected_stored_in_db + + +def test_alerting_field_response_uses_explicit_config_value(): + settings = SettingsStore("general_settings") + settings.load_yaml({"alerting_args": {"budget_alert_ttl": 60}}) + field_info = proxy_server.SlackAlertingArgs.model_fields["budget_alert_ttl"] + + result = proxy_server._alerting_field_response( + settings=settings, + db_values={}, + config_values={"budget_alert_ttl": 60}, + allowed_args={"budget_alert_ttl": "Integer"}, + field_name="budget_alert_ttl", + field_info=field_info, + ) + + assert result.field_name == "budget_alert_ttl" + assert result.field_type == "Integer" + assert result.field_value == 60 + assert result.stored_in_db is False + assert result.source == "config" + diff --git a/ui/litellm-dashboard/src/components/alerting/alerting_settings.integration.test.tsx b/ui/litellm-dashboard/src/components/alerting/alerting_settings.integration.test.tsx new file mode 100644 index 00000000000..ac857b1a8bd --- /dev/null +++ b/ui/litellm-dashboard/src/components/alerting/alerting_settings.integration.test.tsx @@ -0,0 +1,147 @@ +import React from "react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { act, fireEvent, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { renderWithProviders } from "@/../tests/test-utils"; +import AlertingSettings from "./alerting_settings"; + +const { alertingSettingsCall, updateConfigFieldSetting } = vi.hoisted(() => ({ + alertingSettingsCall: vi.fn(), + updateConfigFieldSetting: vi.fn(), +})); + +vi.mock("@/components/networking", () => ({ alertingSettingsCall, updateConfigFieldSetting })); + +vi.mock("@/lib/toast", () => ({ + toast: { error: vi.fn(), success: vi.fn() }, +})); + +const settingsResponse = [ + { + field_name: "slack_alerting", + field_type: "Boolean", + field_value: false, + field_default_value: null, + field_description: "Enable Slack alerting", + stored_in_db: null, + premium_field: false, + }, + { + field_name: "budget_alert_ttl", + field_type: "Integer", + field_value: 60, + field_default_value: 30, + field_description: "Configured threshold", + stored_in_db: true, + premium_field: false, + }, + { + field_name: "outage_alert_ttl", + field_type: "Integer", + field_value: null, + field_default_value: 10, + field_description: "Unset threshold", + stored_in_db: null, + premium_field: false, + }, +]; + +describe("AlertingSettings", () => { + beforeEach(() => { + vi.clearAllMocks(); + alertingSettingsCall.mockResolvedValue(settingsResponse); + updateConfigFieldSetting.mockResolvedValue({}); + }); + + it("does not submit unset fields when updating another alerting setting", async () => { + const user = userEvent.setup(); + renderWithProviders(); + + await screen.findByDisplayValue("60"); + const numberInputs = screen.getAllByRole("spinbutton"); + fireEvent.change(numberInputs[1], { target: { value: "5" } }); + fireEvent.change(numberInputs[1], { target: { value: "" } }); + await user.click(screen.getByRole("switch", { name: "slack_alerting" })); + await user.click(screen.getByRole("button", { name: "Update Settings" })); + + await waitFor(() => { + expect(updateConfigFieldSetting).toHaveBeenNthCalledWith(1, "sk-test", "alerting_args", { + budget_alert_ttl: 60, + }); + }); + expect(updateConfigFieldSetting).toHaveBeenNthCalledWith(2, "sk-test", "alerting", ["slack"]); + }); + + it("does not restore a stale typed value after reset", async () => { + const user = userEvent.setup(); + renderWithProviders(); + + const budgetInput = await screen.findByDisplayValue("60"); + fireEvent.change(budgetInput, { target: { value: "75" } }); + + await user.click(screen.getByRole("button", { name: "Reset budget_alert_ttl" })); + await user.click(screen.getByRole("button", { name: "Update Settings" })); + + await waitFor(() => { + expect(updateConfigFieldSetting).toHaveBeenCalledWith("sk-test", "alerting_args", {}); + }); + }); + + it("clears reset and form state when the access token changes", async () => { + const refreshedSettings = settingsResponse.map((setting) => + setting.field_name === "budget_alert_ttl" ? { ...setting, field_value: 90 } : setting, + ); + alertingSettingsCall.mockImplementation((token: string) => + Promise.resolve(token === "sk-new" ? refreshedSettings : settingsResponse), + ); + + const user = userEvent.setup(); + const { rerender } = renderWithProviders(); + + const budgetInput = await screen.findByDisplayValue("60"); + fireEvent.change(budgetInput, { target: { value: "75" } }); + await user.click(screen.getByRole("button", { name: "Reset budget_alert_ttl" })); + + rerender(); + await screen.findByDisplayValue("90"); + + await user.click(screen.getByRole("switch", { name: "slack_alerting" })); + await user.click(screen.getByRole("button", { name: "Update Settings" })); + + await waitFor(() => { + expect(updateConfigFieldSetting).toHaveBeenNthCalledWith(1, "sk-new", "alerting_args", { + budget_alert_ttl: 90, + }); + }); + expect(updateConfigFieldSetting).toHaveBeenNthCalledWith(2, "sk-new", "alerting", ["slack"]); + }); + + it("ignores an older settings response after the access token changes", async () => { + const oldRequest = Promise.withResolvers(); + const newRequest = Promise.withResolvers(); + const refreshedSettings = settingsResponse.map((setting) => + setting.field_name === "budget_alert_ttl" ? { ...setting, field_value: 90 } : setting, + ); + + alertingSettingsCall.mockImplementation((token: string) => + token === "sk-new" ? newRequest.promise : oldRequest.promise, + ); + + const { rerender } = renderWithProviders(); + rerender(); + + await act(async () => { + newRequest.resolve(refreshedSettings); + await newRequest.promise; + }); + await screen.findByDisplayValue("90"); + + await act(async () => { + oldRequest.resolve(settingsResponse); + await oldRequest.promise; + }); + + expect(screen.getByDisplayValue("90")).toBeInTheDocument(); + expect(screen.queryByDisplayValue("60")).not.toBeInTheDocument(); + }); +}); diff --git a/ui/litellm-dashboard/src/components/alerting/alerting_settings.tsx b/ui/litellm-dashboard/src/components/alerting/alerting_settings.tsx index cff33546fbc..406748a912e 100644 --- a/ui/litellm-dashboard/src/components/alerting/alerting_settings.tsx +++ b/ui/litellm-dashboard/src/components/alerting/alerting_settings.tsx @@ -24,19 +24,32 @@ interface AlertingSettingsProps { const AlertingSettings: React.FC = ({ accessToken, premiumUser }) => { const [alertingSettings, setAlertingSettings] = useState([]); + const [resetFields, setResetFields] = useState>(new Set()); useEffect(() => { - // get values if (!accessToken) { return; } + + const controller = new AbortController(); alertingSettingsCall(accessToken).then((data) => { + if (controller.signal.aborted) { + return; + } + setResetFields(new Set()); setAlertingSettings(data); }); + + return () => { + controller.abort(); + }; }, [accessToken]); const handleInputChange = (fieldName: string, newValue: any) => { - // Update the value in the state + setResetFields((previous) => + previous.has(fieldName) ? new Set([...previous].filter((name) => name !== fieldName)) : previous, + ); + const updatedSettings = alertingSettings.map((setting) => setting.field_name === fieldName ? { ...setting, field_value: newValue } : setting, ); @@ -49,21 +62,32 @@ const AlertingSettings: React.FC = ({ accessToken, premiu return; } - let fieldValue = formValues; + const fieldValue = formValues; if (fieldValue == null || fieldValue == undefined) { return; } - const initialFormValues: Record = {}; + const configuredAlertingArgs: Record = Object.fromEntries( + alertingSettings + .filter( + (setting) => + setting.field_name !== "slack_alerting" && + !resetFields.has(setting.field_name) && + setting.field_value != null, + ) + .map((setting) => [setting.field_name, setting.field_value]), + ); - alertingSettings.forEach((setting) => { - initialFormValues[setting.field_name] = setting.field_value; - }); - - // Merge initialFormValues with actual formValues - const mergedFormValues = { ...formValues, ...initialFormValues }; - const { slack_alerting, ...alertingArgs } = mergedFormValues; + const { slack_alerting, ...updatedAlertingArgs } = formValues; + const alertingArgs = { + ...configuredAlertingArgs, + ...Object.fromEntries( + Object.entries(updatedAlertingArgs).filter( + ([fieldName, value]) => !resetFields.has(fieldName) && value != null && value !== "", + ), + ), + }; try { await updateConfigFieldSetting(accessToken, "alerting_args", alertingArgs); if (typeof slack_alerting === "boolean") { @@ -86,8 +110,7 @@ const AlertingSettings: React.FC = ({ accessToken, premiu } try { - // deleteConfigFieldSetting(accessToken, fieldName); - // update value in state + setResetFields((previous) => new Set([...previous, fieldName])); const updatedSettings = alertingSettings.map((setting) => setting.field_name === fieldName @@ -106,6 +129,7 @@ const AlertingSettings: React.FC = ({ accessToken, premiu return (