diff --git a/CHANGELOG.md b/CHANGELOG.md index 48ce1fe..fc99140 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,12 @@ first. ### Added +- Config files are brought up to date at startup instead of only on first boot. Each mod compares + the file on disk against the keys it knows and writes the missing ones back with their defaults, + keeping every value already in the file, so a server upgrading from 0.1.0 gets the `Attribution` + block in `pulse.json` and `ServiceName` in `pulse-otlp.json` without anyone editing them by hand. + Keys neither mod recognises are named in a warning, since the rewrite drops them. A file that + already holds every key is left alone, modification time included. - Per-mod tick attribution, behind a new `Attribution` block in `pulse.json` and off by default. `pulse_mod_tick_share{modid}` is the fraction of profiled main-thread busy time one mod took over the last burst, `pulse_mod_tick_seconds_total{modid}` the sampled seconds behind it, diff --git a/Pulse.Otlp.Scenarios/OtlpConfigUpgradeScenarios.cs b/Pulse.Otlp.Scenarios/OtlpConfigUpgradeScenarios.cs new file mode 100644 index 0000000..c544f2f --- /dev/null +++ b/Pulse.Otlp.Scenarios/OtlpConfigUpgradeScenarios.cs @@ -0,0 +1,28 @@ +using System.Text.Json.Nodes; +using Atlas.XUnit; +using Xunit; + +namespace Pulse.Otlp.Scenarios; + +/// The same upgrade on the other mod's file: a pulse-otlp.json written before +/// ServiceName existed gets it, and keeps the endpoint the admin pointed it at. The base +/// mod is seeded complete and turned off, because nothing here is about what it serves; the OTLP +/// mod only needs it present, which its modinfo dependency requires anyway. Nothing listens on the +/// configured endpoint, and nothing needs to: an export that cannot connect is swallowed by the +/// SDK's own export thread. +[AtlasDataFiles("data/configupgrade", TargetPath = "ModConfig")] +public class OtlpConfigUpgradeScenarios : AtlasScenarioBase +{ + [AtlasScenario] + public async Task Startup_Fills_ServiceName_IntoAnOlderConfigFile() + { + await World.Ticks(5); + + string path = Path.Combine(World.Api.GetOrCreateDataPath("ModConfig"), "pulse-otlp.json"); + JsonObject config = Assert.IsType(JsonNode.Parse(File.ReadAllText(path))); + + Assert.Equal("vintagestory", (string?)config["ServiceName"]); + Assert.Equal("http://127.0.0.1:39473", (string?)config["Endpoint"]); + Assert.Equal(60, (int)config["IntervalSeconds"]!); + } +} diff --git a/Pulse.Otlp.Scenarios/data/configupgrade/pulse-otlp.json b/Pulse.Otlp.Scenarios/data/configupgrade/pulse-otlp.json new file mode 100644 index 0000000..02b16b4 --- /dev/null +++ b/Pulse.Otlp.Scenarios/data/configupgrade/pulse-otlp.json @@ -0,0 +1,8 @@ +{ + "Enabled": true, + "Endpoint": "http://127.0.0.1:39473", + "Protocol": "http/protobuf", + "Headers": {}, + "IntervalSeconds": 60, + "IncludeRuntimeMetrics": false +} diff --git a/Pulse.Otlp.Scenarios/data/configupgrade/pulse.json b/Pulse.Otlp.Scenarios/data/configupgrade/pulse.json new file mode 100644 index 0000000..433613d --- /dev/null +++ b/Pulse.Otlp.Scenarios/data/configupgrade/pulse.json @@ -0,0 +1,12 @@ +{ + "Enabled": false, + "Bind": "127.0.0.1", + "Port": 39473, + "RuntimeMetrics": false, + "ChunksRefreshSeconds": 30, + "Attribution": { + "Enabled": false, + "BurstTicks": 30, + "IntervalSeconds": 10 + } +} diff --git a/Pulse.Otlp/Pulse.Otlp.csproj b/Pulse.Otlp/Pulse.Otlp.csproj index d5fba3b..1aeb47a 100644 --- a/Pulse.Otlp/Pulse.Otlp.csproj +++ b/Pulse.Otlp/Pulse.Otlp.csproj @@ -16,6 +16,13 @@ + + + + + diff --git a/Pulse.Otlp/PulseOtlpModSystem.cs b/Pulse.Otlp/PulseOtlpModSystem.cs index 70deadb..2ff553e 100644 --- a/Pulse.Otlp/PulseOtlpModSystem.cs +++ b/Pulse.Otlp/PulseOtlpModSystem.cs @@ -29,7 +29,13 @@ public sealed class PulseOtlpModSystem : ModSystem public override void StartServerSide(ICoreServerAPI api) { - PulseOtlpConfig config = api.LoadModConfig(ConfigFile) ?? StoreDefaults(api); + PulseOtlpConfig? existing = api.LoadModConfig(ConfigFile); + PulseOtlpConfig config = existing ?? StoreDefaults(api); + if (existing != null) + { + ConfigUpgrade.Upgrade(api, config, ConfigFile, "Pulse OTLP"); + } + if (!config.Enabled) { api.Logger.Notification("Pulse OTLP is disabled in " + ConfigFile + ", nothing registered."); diff --git a/Pulse.Scenarios/ConfigUpgradeScenarios.cs b/Pulse.Scenarios/ConfigUpgradeScenarios.cs new file mode 100644 index 0000000..145c240 --- /dev/null +++ b/Pulse.Scenarios/ConfigUpgradeScenarios.cs @@ -0,0 +1,44 @@ +using System.Text.Json.Nodes; +using Atlas.XUnit; +using Xunit; + +namespace Pulse.Scenarios; + +/// An admin's own pulse.json, brought up to date by a real server booting on it. The +/// seeded file is what an upgrade actually looks like: one key the admin set, nothing else the +/// current version declares, and one key that answers to nothing. +[AtlasDataFiles("data/configupgrade/pulse.json", TargetPath = "ModConfig")] +public class ConfigUpgradeScenarios : AtlasScenarioBase +{ + private const int Port = 39472; + + [AtlasScenario] + public async Task Startup_Fills_AnOlderConfigFile_WithoutLosingWhatTheAdminSet() + { + await World.Ticks(5); + + string path = Path.Combine(World.Api.GetOrCreateDataPath("ModConfig"), "pulse.json"); + JsonObject config = Assert.IsType(JsonNode.Parse(File.ReadAllText(path))); + + // The admin's one setting, still theirs on disk and still what the endpoint bound: a + // rewrite that reset it to the default would be the worst possible outcome here. + Assert.Equal(Port, (int)config["Port"]!); + Assert.Contains("pulse_server_ticks_total", await Scrape.Metrics(Port)); + + // The block that arrived after 0.1.0, written out with the defaults the class declares. + JsonObject attribution = Assert.IsType(config["Attribution"]); + Assert.False((bool)attribution["Enabled"]!); + Assert.Equal(30, (int)attribution["BurstTicks"]!); + Assert.Equal(10, (int)attribution["IntervalSeconds"]!); + + // And the rest of the keys the file never had. + Assert.True((bool)config["Enabled"]!); + Assert.Equal("127.0.0.1", (string?)config["Bind"]); + Assert.True((bool)config["RuntimeMetrics"]!); + Assert.Equal(30, (int)config["ChunksRefreshSeconds"]!); + + // The key nothing in PulseConfig answers to does not survive the rewrite, which is why the + // mod warns about it rather than dropping it quietly. + Assert.Null(config["Colour"]); + } +} diff --git a/Pulse.Scenarios/data/configupgrade/pulse.json b/Pulse.Scenarios/data/configupgrade/pulse.json new file mode 100644 index 0000000..04758ea --- /dev/null +++ b/Pulse.Scenarios/data/configupgrade/pulse.json @@ -0,0 +1,4 @@ +{ + "Port": 39472, + "Colour": "green" +} diff --git a/Pulse.Tests/ConfigUpgradeTests.cs b/Pulse.Tests/ConfigUpgradeTests.cs new file mode 100644 index 0000000..673cc17 --- /dev/null +++ b/Pulse.Tests/ConfigUpgradeTests.cs @@ -0,0 +1,183 @@ +using Xunit; + +namespace Pulse.Tests; + +/// The comparison an upgrade turns on. Both sides are JSON text here, which is what the +/// two mods hand it: the file as the admin left it, and the config object as it was loaded. +public class ConfigUpgradeTests +{ + /// A 0.1.0 file against a 0.2.0 config: the whole Attribution block arrived in the + /// release the admin is upgrading to. + private const string OldFile = """ + { + "Enabled": true, + "Bind": "127.0.0.1", + "Port": 9464, + "RuntimeMetrics": true, + "ChunksRefreshSeconds": 30 + } + """; + + private const string CurrentConfig = """ + { + "Enabled": true, + "Bind": "127.0.0.1", + "Port": 9464, + "RuntimeMetrics": true, + "ChunksRefreshSeconds": 30, + "Attribution": { "Enabled": false, "BurstTicks": 30, "IntervalSeconds": 10 } + } + """; + + [Fact] + public void Compare_Reports_ABlockTheFilePredates() + { + ConfigDiff diff = ConfigUpgrade.Compare(OldFile, CurrentConfig); + + // The block by its own name, not its three children: the admin never had any of them, and + // naming them would only pad the log line. + Assert.Equal(["Attribution"], diff.Missing); + Assert.Empty(diff.Unknown); + } + + /// The recursive half. A file that has the block but predates one key inside it gets + /// that one key named, and nothing else. + [Fact] + public void Compare_Reports_AKeyMissingFromAPresentBlock_ByItsPath() + { + const string file = """ + { + "Enabled": true, + "Attribution": { "Enabled": true, "IntervalSeconds": 10 } + } + """; + const string config = """ + { + "Enabled": true, + "Attribution": { "Enabled": true, "BurstTicks": 30, "IntervalSeconds": 10 } + } + """; + + ConfigDiff diff = ConfigUpgrade.Compare(file, config); + + Assert.Equal(["Attribution.BurstTicks"], diff.Missing); + Assert.Empty(diff.Unknown); + } + + [Fact] + public void Compare_Reports_AKeyTheConfigDoesNotKnow_AtEitherDepth() + { + const string file = """ + { + "Enabled": true, + "Colour": "green", + "Attribution": { "Enabled": true, "Burstticks": 5 } + } + """; + const string config = """ + { + "Enabled": true, + "Attribution": { "Enabled": true, "BurstTicks": 30 } + } + """; + + ConfigDiff diff = ConfigUpgrade.Compare(file, config); + + Assert.Equal(["Attribution.BurstTicks"], diff.Missing); + Assert.Equal(["Colour", "Attribution.Burstticks"], diff.Unknown); + } + + /// Key order and whitespace are the serializer's business, not the admin's, and a + /// reordered file must not read as an upgrade. + [Fact] + public void Compare_Ignores_KeyOrderAndFormatting() + { + const string file = """{"Port":9464,"Bind":"127.0.0.1","Attribution":{"BurstTicks":30,"Enabled":false}}"""; + const string config = """ + { + "Bind": "127.0.0.1", + "Port": 9464, + "Attribution": { + "Enabled": false, + "BurstTicks": 30 + } + } + """; + + ConfigDiff diff = ConfigUpgrade.Compare(file, config); + + Assert.Empty(diff.Missing); + Assert.Empty(diff.Unknown); + } + + /// The admin's own settings are the whole point of the exercise: a file that differs + /// from the defaults in every value is complete, not out of date. + [Fact] + public void Compare_Ignores_Values() + { + const string file = """ + { + "Enabled": false, + "Bind": "0.0.0.0", + "Port": 19464, + "Attribution": { "Enabled": true, "BurstTicks": 5 } + } + """; + const string config = """ + { + "Enabled": true, + "Bind": "127.0.0.1", + "Port": 9464, + "Attribution": { "Enabled": false, "BurstTicks": 30 } + } + """; + + ConfigDiff diff = ConfigUpgrade.Compare(file, config); + + Assert.Empty(diff.Missing); + Assert.Empty(diff.Unknown); + } + + /// A block that is not a block on disk stops the walk there. Whatever the admin put + /// in its place is theirs, and the rewrite replaces the lot with one default block. + [Fact] + public void Compare_Stops_AtAKeyThatIsAnObjectOnOnlyOneSide() + { + ConfigDiff diff = ConfigUpgrade.Compare("""{"Attribution": true}""", """{"Attribution": {"Enabled": false}}"""); + + Assert.Empty(diff.Missing); + Assert.Empty(diff.Unknown); + } + + /// Newtonsoft loads a file with comments and trailing commas without complaint, so a + /// server running on one must not silently stop getting new keys. + [Fact] + public void Compare_Reads_AFileWithCommentsAndATrailingComma() + { + const string file = """ + { + // the port the panel scrapes + "Port": 9464, + } + """; + + ConfigDiff diff = ConfigUpgrade.Compare(file, """{"Port": 9464, "Bind": "127.0.0.1"}"""); + + Assert.Equal(["Bind"], diff.Missing); + Assert.Empty(diff.Unknown); + } + + /// Nothing missing means nothing to write, and text that is not a JSON object at all + /// has to land there too: rewriting a file this cannot read would destroy it. + [Theory] + [InlineData("not json at all")] + [InlineData("[1, 2, 3]")] + [InlineData("")] + public void Compare_Reports_Nothing_ForTextThatIsNotAJsonObject(string file) + { + ConfigDiff diff = ConfigUpgrade.Compare(file, """{"Port": 9464}"""); + + Assert.Empty(diff.Missing); + Assert.Empty(diff.Unknown); + } +} diff --git a/Pulse/ConfigUpgrade.cs b/Pulse/ConfigUpgrade.cs new file mode 100644 index 0000000..f849fa2 --- /dev/null +++ b/Pulse/ConfigUpgrade.cs @@ -0,0 +1,133 @@ +using System.Text.Json; +using System.Text.Json.Nodes; +using Vintagestory.API.Common; +using Vintagestory.API.Server; + +namespace Pulse; + +/// Keys the config file on disk does not carry, and keys it carries that the mod knows +/// nothing about. Dotted paths, so a key inside a nested block reads +/// Attribution.BurstTicks. +internal readonly record struct ConfigDiff(IReadOnlyList Missing, IReadOnlyList Unknown); + +/// Keeps the config file an admin already has in step with the keys a newer version of the +/// mod introduced. +/// Both mods compile this from the one source file: Pulse.Otlp links it rather than +/// referencing Pulse.dll, because the two mods deliberately share no assembly. +internal static class ConfigUpgrade +{ + private const string AddedKeys = + "{0} added these keys to {1} with their defaults: {2}. Everything already in the file was " + + "kept as it was."; + + private const string DroppedKeys = + "{0} does not know these keys in {1}, and rewriting the file has just dropped them: {2}. " + + "Check them for typos."; + + private const string IgnoredKeys = + "{0} does not know these keys in {1}: {2}. They do nothing; check them for typos."; + + private const string UpgradeFailed = + "{0} could not bring {1} up to date ({2}). The server runs on the values the file does " + + "have, with defaults for the rest."; + + /// Newtonsoft accepts both when it loads the config, so a file the game read happily + /// must not be one this refuses to look at. + private static readonly JsonDocumentOptions Lenient = + new() { CommentHandling = JsonCommentHandling.Skip, AllowTrailingCommas = true }; + + /// Adds whatever keys a newer version of the mod introduced to the config file the + /// admin already has, and says in the log what changed. + /// The file is rewritten from , which the loader filled with + /// defaults wherever the file was silent, so the write only ever adds: every value the admin + /// set is already in the object. It happens solely when a key is missing, because a complete + /// file must not be touched at all, not even its modification time, on a host that mounts + /// ModConfig read-only or tracks it. The same rewrite is what drops a key the config does not + /// know, which is why an unknown key is worth a warning rather than silence. + /// Nothing in here may take a server down over a config file, so a read or a write that + /// fails is one warning and then the server carries on unchanged. + public static void Upgrade(ICoreServerAPI api, T config, string filename, string modName) + where T : class + { + ConfigDiff diff; + try + { + // ToPrettyString is the very call StoreModConfig makes to write the file: Newtonsoft + // with no settings at all, so the keys compared here are the keys a rewrite produces. + string path = Path.Combine(api.GetOrCreateDataPath("ModConfig"), filename); + diff = Compare(File.ReadAllText(path), JsonUtil.ToPrettyString(config)); + if (diff.Missing.Count > 0) + { + api.StoreModConfig(config, filename); + api.Logger.Notification(AddedKeys, modName, filename, string.Join(", ", diff.Missing)); + } + } + catch (Exception e) + { + api.Logger.Warning(UpgradeFailed, modName, filename, e.Message); + return; + } + + if (diff.Unknown.Count > 0) + { + api.Logger.Warning( + diff.Missing.Count > 0 ? DroppedKeys : IgnoredKeys, + modName, filename, string.Join(", ", diff.Unknown)); + } + } + + /// Which keys of are absent from , + /// and which keys of are absent from . + /// Keys only, never values, so key order and formatting make no difference. A missing + /// block is reported by its own name and not walked: naming its children would only pad the log + /// line with keys the admin never had. Text that does not parse as a JSON object reports + /// nothing, which leaves the file alone rather than rewriting something unreadable. + public static ConfigDiff Compare(string onDisk, string loaded) + { + List missing = []; + List unknown = []; + if (Parse(onDisk) is { } file && Parse(loaded) is { } config) + { + Walk(file, config, string.Empty, missing, unknown); + } + + return new ConfigDiff(missing, unknown); + } + + private static void Walk( + JsonObject file, JsonObject config, string prefix, List missing, List unknown) + { + // This level before the blocks under it, so both lists read outermost key first. + foreach (KeyValuePair entry in file) + { + if (!config.ContainsKey(entry.Key)) + { + unknown.Add(prefix + entry.Key); + } + } + + foreach (KeyValuePair entry in config) + { + if (!file.TryGetPropertyValue(entry.Key, out JsonNode? theirs)) + { + missing.Add(prefix + entry.Key); + } + else if (entry.Value is JsonObject nested && theirs is JsonObject nestedFile) + { + Walk(nestedFile, nested, prefix + entry.Key + ".", missing, unknown); + } + } + } + + private static JsonObject? Parse(string json) + { + try + { + return JsonNode.Parse(json, nodeOptions: null, Lenient) as JsonObject; + } + catch (JsonException) + { + return null; + } + } +} diff --git a/Pulse/PulseModSystem.cs b/Pulse/PulseModSystem.cs index 19d60d8..d8384a8 100644 --- a/Pulse/PulseModSystem.cs +++ b/Pulse/PulseModSystem.cs @@ -103,7 +103,13 @@ public override void StartServerSide(ICoreServerAPI api) { sapi = api; - PulseConfig config = api.LoadModConfig(ConfigFile) ?? StoreDefaults(api); + PulseConfig? existing = api.LoadModConfig(ConfigFile); + PulseConfig config = existing ?? StoreDefaults(api); + if (existing != null) + { + ConfigUpgrade.Upgrade(api, config, ConfigFile, "Pulse"); + } + if (!config.Enabled) { api.Logger.Notification("Pulse is disabled in " + ConfigFile + ", nothing registered."); diff --git a/README.md b/README.md index 3b2da23..2ced3d6 100644 --- a/README.md +++ b/README.md @@ -220,6 +220,14 @@ costs; lower it only if you know why. `Attribution` is the per-mod breakdown des because it costs tick time; with it off, nothing in that section is registered and the engine's profiler is never touched. Every one of these takes a server restart. +Upgrading does not mean editing the file by hand. Each mod checks its config file at startup and +writes back any key it knows about that the file is missing, with that key's default; the values +you already set are kept exactly as they are, and the log lists what was added. A key neither mod +recognises does not survive that rewrite, so it is reported as a warning instead of disappearing +quietly: usually it is a typo, and the setting you meant has been running on its default. A file +that already holds every key is not written at all, which matters if you mount `ModConfig` +read-only or keep it under version control. + ## Scraping it ```yaml diff --git a/tools/mutation-check.sh b/tools/mutation-check.sh index c7c9876..5cac9b2 100755 --- a/tools/mutation-check.sh +++ b/tools/mutation-check.sh @@ -47,7 +47,7 @@ mutate() { #