Removed deprecated options, updated docker samples, chrore telemetry #31
Conversation
WalkthroughRenamed compose/env variables; renamed endpoint APIs and updated tests to replication/node terminology; expanded ClusterVNodeOptions (many new option groups and certificate-store loading); TelemetryService reads IConfiguration and publishes config-driven telemetry; minor auth diagnostics and DriveStats/logging changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant VNode as ClusterVNode
participant Telemetry as TelemetryService
participant Cfg as IConfiguration
participant Pub as IPublisher
participant Sink as ITelemetrySink
VNode->>Telemetry: new TelemetryService(manager, options, configuration, publisher, sink, checkpoint, nodeId)
note right of Telemetry #F3F7EA: stores IConfiguration for extra telemetry
Pub-->>Telemetry: PluginTelemetry(evt)
Telemetry->>Telemetry: Normalize keys (lower-first-letter)
Telemetry->>Sink: Publish envelope { root: LowerFirstLetter(evt.Source), data }
Telemetry->>Cfg: Read "EventStore:Telemetry"
Cfg-->>Telemetry: Key/Value map
Telemetry->>Telemetry: Normalize keys
Telemetry->>Sink: Publish envelope { root: "telemetry", data }
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
j-fuxa
force-pushed
the
docker_telemetry
branch
from
41b0d75 to
12a0c46
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (16)
src/EventStore.Core/Authentication/PassthroughAuthentication/PassthroughAuthenticationProvider.cs (1)
15-15: Avoid per-call allocation for supported schemes (minor)
["Insecure"]creates a new array on each call. If this is on a hot path, return a cached static instead.Apply within this line:
-public override IReadOnlyList<string> GetSupportedAuthenticationSchemes() => ["Insecure"]; +public override IReadOnlyList<string> GetSupportedAuthenticationSchemes() => Schemes;And add this class-scoped field (outside the selected line range):
private static readonly string[] Schemes = ["Insecure"];src/EventStore.Core/Telemetry/TelemetryService.cs (2)
269-276: Emit “telemetry” only when there’s content; also normalize safelyTwo tweaks:
- If EventStore:Telemetry is empty, skip emitting an empty “telemetry” object.
- Same collision risk as above; avoid ToDictionary to prevent exceptions on case-only duplicates.
- var extraTelemetry = _configuration.GetSection("EventStore:Telemetry").Get<Dictionary<string, string>>() ?? - []; - var payload = - JsonSerializer.SerializeToNode(extraTelemetry.ToDictionary(kvp => LowerFirstLetter(kvp.Key), - kvp => kvp.Value)); - message.Envelope.ReplyWith(new TelemetryMessage.Response( - "telemetry", payload)); + var configured = _configuration.GetSection("EventStore:Telemetry") + .Get<Dictionary<string, string>>() ?? new(); + if (configured.Count > 0) { + var normalized = new Dictionary<string, string>(StringComparer.Ordinal); + foreach (var kvp in configured) + normalized[LowerFirstLetter(kvp.Key)] = kvp.Value; + var payload = JsonSerializer.SerializeToNode(normalized); + message.Envelope.ReplyWith(new TelemetryMessage.Response("telemetry", payload)); + }
269-276: Security/PII: guard against sensitive keys in EventStore:TelemetryConfig-driven telemetry is powerful, but it can leak secrets if users set keys like "password/secret/token/apiKey". Consider masking known-sensitive suffixes/patterns before publishing.
I can add a small filter like: if (key.Contains("token", OrdinalIgnoreCase) || key.Contains("secret", ...)) value = "****".
Do you want me to wire this in with a unit test?
src/EventStore.Core.XUnit.Tests/Telemetry/TelemetryServiceTests.cs (3)
75-95: Use UTC time for deterministic testsPrefer DateTime.UtcNow to avoid timezone-dependent behavior and daylight-saving edge cases in future assertions or logs.
Apply this diff:
- timeStamp: DateTime.Now, + timeStamp: DateTime.UtcNow,
137-144: Avoid brittle JSON string comparisons; assert on semantic JSON valuesComparing JsonObject.ToString() to a multi-line literal is formatting-sensitive. Assert on the actual JSON value(s) instead.
Apply this diff:
- Assert.Equal(""" - { - "baz": "qux" - } - """, - _sink.Data["fakeComponent"].ToString()); + Assert.Equal("qux", _sink.Data["fakeComponent"]!["baz"]!.GetValue<string>());
147-154: Same here: assert on telemetry value rather than full JSON textThis avoids failures due to whitespace, ordering, or serializer differences.
Apply this diff:
- Assert.Equal(""" - { - "cloudIdentifier": "abc" - } - """, - _sink.Data["telemetry"].ToString()); + Assert.Equal("abc", _sink.Data["telemetry"]!["cloudIdentifier"]!.GetValue<string>());src/EventStore.Core/Configuration/ClusterVNodeOptions.cs (3)
32-33: Avoid side effects in options constructorCalling FileStreamExtensions.ConfigureFlush in the parameterless constructor couples object creation to a global side effect using default values, and won’t reflect later user-provided overrides (e.g., when UnsafeDisableFlushToDisk is set via configuration after construction).
Move this to the end of FromConfiguration(...) after binding, so the configured value is applied exactly once per load:
-public ClusterVNodeOptions() => FileStreamExtensions.ConfigureFlush(Database.UnsafeDisableFlushToDisk); +public ClusterVNodeOptions() {}And at the end of FromConfiguration (just before returning):
- return options; + FileStreamExtensions.ConfigureFlush(options.Database.UnsafeDisableFlushToDisk); + return options;
60-93: Apply flush configuration after bindingFollow-up to the previous comment: ensure the flush behavior respects configured values by applying it here, post-bind.
Apply this diff at the end of the method:
var options = new ClusterVNodeOptions { // ... existing bindings ... LoadedOptions = GetLoadedOptions(configurationRoot) }; - return options; + FileStreamExtensions.ConfigureFlush(options.Database.UnsafeDisableFlushToDisk); + return options;
563-567: Use central Opts defaults to avoid duplicationThese defaults are already defined in Opts. Reuse them to keep a single source of truth.
Apply this diff:
- public int ConnectionPendingSendBytesThreshold { get; init; } = 10 * 1_024 * 1_024; + public int ConnectionPendingSendBytesThreshold { get; init; } = Opts.ConnectionPendingSendBytesThresholdDefault; @@ - public int ConnectionQueueSizeThreshold { get; init; } = 50_000; + public int ConnectionQueueSizeThreshold { get; init; } = Opts.ConnectionQueueSizeThresholdDefault;src/EventStore.Core.Tests/Helpers/MiniNode.cs (1)
181-182: AdvertiseNodeAs gating is sensibleOnly applying advertisement when a host is provided keeps behavior backward compatible. Consider also guarding for whitespace-only strings, though not critical for tests.
src/EventStore.Core/Configuration/ClusterVNodeOptionsExtensions.cs (4)
138-141: Clarify AdvertiseExternalHostAs semantics (host-only) and relation to AdvertiseNodeAsThis method sets NodeHostAdvertiseAs only. That’s consistent with later using AdvertiseNodeAs to set the HTTP port. Please make this explicit in the XML docs to avoid confusion when teams expect this to also set the port.
Apply this doc-only diff:
/// <summary> -/// Sets up the External Host that would be advertised +/// Sets the external host (host only) that will be advertised to clients/gossip. +/// Note: This does not set the HTTP port; use <see cref="AdvertiseNodeAs"/> to set both host and port. /// </summary>
159-171: Fill in missing XML docs for AdvertiseNodeAsThe summary/param docs are empty. This API becomes the primary way to set the advertised HTTP endpoint (host+port) and deserves explicit documentation.
Apply this doc-only diff:
/// <summary> -/// +/// Sets the advertised node HTTP endpoint (host + port) used by clients and gossip. /// </summary> -/// <param name="options">The <see cref="ClusterVNodeOptions"/></param> -/// <param name="endPoint">The advertised host</param> +/// <param name="options">The <see cref="ClusterVNodeOptions"/> to update.</param> +/// <param name="endPoint">The endpoint whose host and port will be advertised.</param> /// <returns>A <see cref="ClusterVNodeOptions"/> with the options set</returns>
255-273: Pre-validate TrustedRootCertificatesPath to improve error signalWhen using the filesystem path, pre-check for directory existence to fail fast with a clearer message before attempting enumeration.
Apply this diff:
- if (string.IsNullOrEmpty(options.Certificate.TrustedRootCertificatesPath)) + if (string.IsNullOrEmpty(options.Certificate.TrustedRootCertificatesPath)) { throw new InvalidConfigurationException( $"{nameof(options.Certificate.TrustedRootCertificatesPath)} must be specified unless insecure mode (--insecure) is set."); } Log.Information("Loading trusted root certificates."); - foreach (var (fileName, cert) in CertificateUtils + var rootsPath = options.Certificate.TrustedRootCertificatesPath; + if (!System.IO.Directory.Exists(rootsPath)) + throw new InvalidConfigurationException( + $"TrustedRootCertificatesPath does not exist: {rootsPath}"); + + foreach (var (fileName, cert) in CertificateUtils - .LoadAllCertificates(options.Certificate.TrustedRootCertificatesPath)) + .LoadAllCertificates(rootsPath)) { trustedRootCerts.Add(cert); Log.Information("Loading trusted root certificate file: {file}", fileName); } if (trustedRootCerts.Count == 0) throw new InvalidConfigurationException( - $"No trusted root certificate files were loaded from the specified path: {options.Certificate.TrustedRootCertificatesPath}"); + $"No trusted root certificate files were loaded from the specified path: {rootsPath}"); return trustedRootCerts;
41-44: Minor: avoid constructing a new ClusterVNodeOptions to get the default DB pathRunInMemory sets Db to new ClusterVNodeOptions().Database.Db. If there’s a canonical default value, consider centralizing it (e.g., a static readonly on DatabaseOptions) to avoid allocating and to make the default explicit.
src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_single_node_and_custom_settings.cs (2)
60-63: API rename usage LGTM; add a test to assert external TCP mapping explicitlyThe migration to WithNodeEndpointOn/WithExternalTcpOn/WithReplicationEndpointOn looks correct. To catch the potential NodeIp overwrite discussed in the core extensions, add an explicit assertion for the external TCP endpoint and a scenario where HTTP and external TCP use different IPs.
Apply this test augmentation (safe if NodeInfo.ExternalSecureTcp exists):
[Test] public void should_set_internal_tcp_endpoint() { Assert.AreEqual(_internalTcp, _node.NodeInfo.InternalSecureTcp); } + + [Test] + public void should_set_external_tcp_endpoint() + { + Assert.AreEqual(_externalTcp, _node.NodeInfo.ExternalSecureTcp); + }Optionally, add a divergence scenario to ensure WithExternalTcpOn does not overwrite the HTTP endpoint IP:
// New test fixture in the same file [TestFixture(typeof(LogFormat.V2), typeof(string))] [TestFixture(typeof(LogFormat.V3), typeof(uint))] public class with_diverging_node_and_external_ips<TLogFormat, TStreamId> : SingleNodeScenario<TLogFormat, TStreamId> { private readonly IPEndPoint _httpEndPoint = new(IPAddress.Parse("127.1.0.1"), 1113); private readonly IPEndPoint _externalTcp = new(IPAddress.Parse("127.2.0.2"), 1115); private readonly IPEndPoint _internalTcp = new(IPAddress.Parse("127.0.0.1"), 1114); protected override ClusterVNodeOptions WithOptions(ClusterVNodeOptions options) => options.WithNodeEndpointOn(_httpEndPoint) .WithExternalTcpOn(_externalTcp) .WithReplicationEndpointOn(_internalTcp); [Test] public void http_endpoint_should_remain_configured() => Assert.AreEqual(_httpEndPoint, _node.NodeInfo.HttpEndPoint); [Test] public void external_tcp_should_match_provided_endpoint() => Assert.AreEqual(_externalTcp, _node.NodeInfo.ExternalSecureTcp); }
157-163: LGTM: Consolidated advertise API usageCombining AdvertiseInternalHostAs (internal host+port) with AdvertiseNodeAs (HTTP host+port) is clear and mirrors the new split between replication and node endpoints.
Consider adding a companion test to confirm call ordering does not matter (i.e., calling AdvertiseNodeAs before AdvertiseExternalHostAs yields the same outcome). This will protect against future regressions in builder composition.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
docker-compose.yml(3 hunks)samples/server/docker-compose.yaml(1 hunks)shared.env(1 hunks)src/EventStore.Core.Tests/Helpers/MiniClusterNode.cs(0 hunks)src/EventStore.Core.Tests/Helpers/MiniNode.cs(2 hunks)src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_cluster_node_and_custom_settings.cs(4 hunks)src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_default_settings.cs(0 hunks)src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_secure_tcp.cs(3 hunks)src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_single_node_and_custom_settings.cs(2 hunks)src/EventStore.Core.XUnit.Tests/Telemetry/TelemetryServiceTests.cs(9 hunks)src/EventStore.Core/Authentication/PassthroughAuthentication/PassthroughAuthenticationProvider.cs(1 hunks)src/EventStore.Core/ClusterVNode.cs(1 hunks)src/EventStore.Core/Configuration/ClusterVNodeOptions.cs(1 hunks)src/EventStore.Core/Configuration/ClusterVNodeOptionsExtensions.cs(1 hunks)src/EventStore.Core/Telemetry/TelemetryService.cs(7 hunks)
💤 Files with no reviewable changes (2)
- src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_default_settings.cs
- src/EventStore.Core.Tests/Helpers/MiniClusterNode.cs
🧰 Additional context used
🧬 Code graph analysis (5)
src/EventStore.Core/Authentication/PassthroughAuthentication/PassthroughAuthenticationProvider.cs (1)
src/EventStore.Core/Services/Transport/Http/Authentication/PassthroughHttpAuthenticationProvider.cs (1)
PassthroughAuthenticationProvider(17-23)
src/EventStore.Core/Telemetry/TelemetryService.cs (2)
src/EventStore.Core/Messaging/IEnvelope.cs (1)
ReplyWith(3-3)src/EventStore.Core/Telemetry/TelemetryMessage.cs (2)
Response(26-34)Response(36-37)
src/EventStore.Core.XUnit.Tests/Telemetry/TelemetryServiceTests.cs (1)
src/EventStore.SystemRuntime/RuntimeInformation.cs (1)
RuntimeInformation(15-50)
src/EventStore.Core/Configuration/ClusterVNodeOptions.cs (7)
src/EventStore.Core/Configuration/ClusterVNodeOptionsExtensions.cs (16)
ClusterVNodeOptions(16-19)ClusterVNodeOptions(21-23)ClusterVNodeOptions(25-34)ClusterVNodeOptions(41-44)ClusterVNodeOptions(52-55)ClusterVNodeOptions(62-67)ClusterVNodeOptions(76-82)ClusterVNodeOptions(90-97)ClusterVNodeOptions(105-107)ClusterVNodeOptions(115-120)ClusterVNodeOptions(128-130)ClusterVNodeOptions(138-140)ClusterVNodeOptions(148-157)ClusterVNodeOptions(164-171)X509Certificate2(178-216)X509Certificate2Collection(227-273)src/EventStore.Core/Configuration/EventStoreConfiguration.cs (3)
IConfigurationRoot(13-54)IConfigurationRoot(56-56)Path(58-72)src/EventStore.Core/Index/PTable.cs (1)
PTableVersions(22-35)src/EventStore.Core/Configuration/TypeConverters.cs (3)
EndPoint(19-31)GossipSeedConverter(34-57)IPAddressConverter(59-67)src/EventStore.Core/TransactionLog/Chunks/TFConsts.cs (1)
TFConsts(4-17)src/EventStore.Core/Util/Opts.cs (1)
Opts(7-33)src/EventStore.Core/Configuration/Sources/EventStoreConfigurationKeys.cs (4)
EventStoreConfigurationKeys(13-94)EventStoreConfigurationKeys(26-40)Normalize(43-72)StripConfigurationPrefix(92-93)
src/EventStore.Core/Configuration/ClusterVNodeOptionsExtensions.cs (7)
src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_cluster_node_and_custom_settings.cs (9)
ClusterVNodeOptions(19-26)ClusterVNodeOptions(87-88)ClusterVNodeOptions(109-110)ClusterVNodeOptions(129-137)ClusterVNodeOptions(157-164)ClusterVNodeOptions(185-192)ClusterVNodeOptions(213-222)ClusterVNodeOptions(246-250)ClusterVNodeOptions(265-268)src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_secure_tcp.cs (2)
ClusterVNodeOptions(21-33)ClusterVNodeOptions(67-73)src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_single_node_and_custom_settings.cs (3)
ClusterVNodeOptions(20-20)ClusterVNodeOptions(39-40)ClusterVNodeOptions(57-63)src/EventStore.Core/Configuration/ClusterVNodeOptions.cs (2)
ClusterVNodeOptions(32-32)ClusterVNodeOptions(60-93)src/EventStore.Common/Utils/EndpointExtensions.cs (2)
GetHost(23-31)GetPort(43-50)src/EventStore.Core/Certificates/CertificateUtils.cs (1)
CertificateUtils(13-315)src/EventStore.Common/Utils/StringExtensions.cs (1)
IsNotEmptyString(14-17)
🔇 Additional comments (22)
src/EventStore.Core/Authentication/PassthroughAuthentication/PassthroughAuthenticationProvider.cs (2)
9-11: Telemetry diagnostics name added — good improvementPassing
diagnosticsName: "PassthroughAuthentication"to the base constructor improves observability and aligns with the PR’s telemetry objectives. No functional concerns here.
9-11: C# 12 Primary Constructor Support ConfirmedThe repo’s
src/Directory.Build.propssets
<TargetFramework>net8.0</TargetFramework><LangVersion>12.0</LangVersion>This guarantees C# 12 support across all projects, so the primary‐constructor syntax in
PassthroughAuthenticationProvideris valid and no changes are required.shared.env (1)
7-7: LGTM: AtomPub toggle preservedKeeping EVENTSTORE_ENABLE_ATOM_PUB_OVER_HTTP=true is consistent with the new naming and expected defaults for samples. No concerns here.
samples/server/docker-compose.yaml (1)
10-14: Confirmed: EVENTSTORE_NODE_PORT is supported in 24.2.0-jammyI verified that the “NodePort” setting is exposed via the environment variable EVENTSTORE_NODE_PORT in the v24.2 Networking documentation, with a default of 2113 and the same 2113:2113 port mapping in Docker Compose (developers.eventstore.com). The previous
HttpPortparameter (and its env var EVENTSTORE_HTTP_PORT) was deprecated in version 23.10.0 when the rename toNodePort/EVENTSTORE_NODE_PORT was introduced (developers.eventstore.com). No changes are required for the 24.2.0-jammy tag.docker-compose.yml (2)
48-61: Same consistency holds for node2; LGTMThe replication IP and advertised node port are aligned with node2’s host mapping (2112). No issues.
70-82: Same consistency holds for node3; LGTMReplication IP and advertised node port align with 2113 mapping. Looks good.
src/EventStore.Core/Telemetry/TelemetryService.cs (2)
42-71: Injecting IConfiguration into TelemetryService is the right move — call sites verifiedAll usages have been updated to include the new
IConfigurationparameter:• ClusterVNode.cs (line 1469)
• TelemetryServiceTests.cs (line 49)No further changes required.
248-265: Telemetry payload grouping change verified in code/testsI searched across C#, JSON, and MD files for patterns like
"plugins":or standalonepluginsin sinks, tests, and docs and found no internal consumers or tests assuming a top-level"plugins"grouping for telemetry messages. Internally,TelemetryServicenow emits each plugin’s snapshot asTelemetryMessage.Response(lowerFirst(evt.Source), payload)instead of nesting under
"plugins". Please manually verify that any external telemetry consumers (sinks, dashboards, alerting rules) and all related documentation have been updated to handle this new top-level key format.src/EventStore.Core/ClusterVNode.cs (1)
1469-1477: LGTM: TelemetryService now receives IConfigurationThe additional configuration parameter is correctly threaded through. The lifetime is managed by registering telemetryService as a singleton for disposal. No issues spotted.
src/EventStore.Core.XUnit.Tests/Telemetry/TelemetryServiceTests.cs (1)
49-55: Constructor update to pass IConfiguration looks goodInjecting IConfiguration and asserting cloudIdentifier via in-memory config is correct and keeps the test focused on TelemetryService behavior. Nice.
src/EventStore.Core/Configuration/ClusterVNodeOptions.cs (1)
511-515: Clarify parameter name in GrpcOptions.FromConfigurationI wasn’t able to find any invocations of
GrpcOptions.FromConfiguration(...)in the current codebase, so it’s unclear whether callers are passing the rootIConfigurationor the specific GRPC section. To make the intent crystal-clear and reduce the risk of misuse, rename the parameter to emphasize that it must be anIConfigurationSection:--- a/src/EventStore.Core/Configuration/ClusterVNodeOptions.cs +++ b/src/EventStore.Core/Configuration/ClusterVNodeOptions.cs @@ -511,7 +511,7 @@ - internal static GrpcOptions FromConfiguration(IConfiguration configurationRoot) => new() + internal static GrpcOptions FromConfiguration(IConfiguration section) => new() { - KeepAliveInterval = configurationRoot.GetValue<int>(nameof(KeepAliveInterval)), - KeepAliveTimeout = configurationRoot.GetValue<int>(nameof(KeepAliveTimeout)) + KeepAliveInterval = section.GetValue<int>(nameof(KeepAliveInterval)), + KeepAliveTimeout = section.GetValue<int>(nameof(KeepAliveTimeout)) };Please verify:
- That all existing callers invoke this method with
configurationRoot.GetSection("EventStore:Grpc")(or similar), not with the full root.- There are no remaining direct passes of the root
IConfigurationinto this method.src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_cluster_node_and_custom_settings.cs (5)
135-137: API rename adoption looks correctSwapping WithInternalTcpOn/WithHttpOn for WithReplicationEndpointOn/WithNodeEndpointOn and using AdvertiseNodeAs aligns with the new naming. Assertions match the expected gossip advertise endpoints.
161-164: Binding Any for replication endpoint is fineUsing IPAddress.Any here is consistent with selecting a non-loopback advertise host in the assertions.
189-192: Node endpoint rename is applied correctlyWithNodeEndpointOn replaces the previous HTTP builder; the subsequent assertion on HttpEndPoint reflects the configured port.
218-222: Mixed Any/Loopback + explicit AdvertiseNodeAs is coherentThe combination of Any for replication and explicit AdvertiseNodeAs for HTTP is consistent. Tests verify the resulting advertised endpoints; looks good.
247-250: New DefaultOpsPassword is exercisedGood to see environment-only defaults covered in tests along with DefaultAdminPassword.
src/EventStore.Core.Tests/Helpers/MiniNode.cs (1)
166-169: Endpoint rename usage LGTMSwitching to WithReplicationEndpointOn/WithExternalTcpOn/WithNodeEndpointOn matches the new API and maintains the same semantics for test setup.
src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_secure_tcp.cs (3)
24-25: TLS scenarios updated to new endpoint APIsUsing WithReplicationEndpointOn/WithExternalTcpOn with CertificateFile mirrors the previous secure-TCP intent. The internal secure endpoint assertion remains valid.
70-73: Secure(...) + new endpoints works as intendedCertificates are supplied via Secure(...) and the replication/external endpoints are set via the new APIs. Assertions are consistent.
104-106: Negative case still validates certificate requirementExpecting an exception without certificates while configuring endpoints is the right guardrail test.
src/EventStore.Core/Configuration/ClusterVNodeOptionsExtensions.cs (2)
148-157: LGTM: Internal advertise maps host+port correctlyAdvertiseInternalHostAs configures ReplicationHostAdvertiseAs and ReplicationTcpPortAdvertiseAs together. This is consistent and avoids the split-host/port confusion.
25-34: LGTM: Builder API changes read clean and preserve semanticsReload, pluggable component aggregation, cluster sizing, in-memory/on-disk toggles, Insecure/Secure paths, gossip seeds, replication and node endpoint setters look cohesive and predictable. Nice use of records/with-expressions.
Also applies to: 41-44, 52-55, 62-68, 76-83, 90-97, 115-121, 128-131
| EVENTSTORE_REPLICATION_PORT=1112 | ||
| EVENTSTORE_NODE_PORT=2113 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Env var rename: replication/http → replication/node; verify image support
The renames to EVENTSTORE_REPLICATION_PORT and EVENTSTORE_NODE_PORT look aligned with the codebase-wide shift to replication/node semantics. Please double-check that the Docker images used in this repo (and in your deployment pipelines) recognize these env vars to avoid silent misconfigurations.
Run this from the repo root to spot usages and ensure these are the canonical names in the codebase:
If you're relying on the prebuilt docker image tag in samples (eventstore/eventstore:24.2.0-jammy), confirm that tag supports these new names in its docs/changelog.
🏁 Script executed:
#!/bin/bash
# Verify env var names are recognized by the server's option binding
rg -nP '\bEVENTSTORE_(REPLICATION|NODE)_(PORT|IP)\b|AdvertiseNodePortToClientAs|NodePortAdvertiseAs|ReplicationPort\b'
# Also list any leftover deprecated names to catch stragglers
rg -nP '\bEVENTSTORE_(INT(_TCP)?|HTTP)_'Length of output: 4161
Verify env var usage and update deprecated samples
The codebase correctly recognizes EVENTSTORE_REPLICATION_PORT and EVENTSTORE_NODE_PORT, but there are still deprecated variables in one of the sample manifests that need updating to prevent misconfiguration:
• In samples/server/docker-compose-cluster.yaml, replace all occurrences of
EVENTSTORE_INT_IP=…with
EVENTSTORE_REPLICATION_IP=…(lines 25, 57, and 82).
Additionally, please confirm that the Docker image tag you’re using (e.g. eventstore/eventstore:24.2.0-jammy) supports the new environment variable names—EVENTSTORE_REPLICATION_PORT and EVENTSTORE_NODE_PORT—in its documentation or changelog to avoid silent mismatches.
🤖 Prompt for AI Agents
In shared.env around lines 3-4, the sample manifests still use the deprecated
EVENTSTORE_INT_IP variable; open samples/server/docker-compose-cluster.yaml and
replace all occurrences of EVENTSTORE_INT_IP with EVENTSTORE_REPLICATION_IP at
lines 25, 57, and 82, update any related comments to reflect the new name, and
verify the docker image tag used (e.g., eventstore/eventstore:24.2.0-jammy)
supports EVENTSTORE_REPLICATION_PORT and EVENTSTORE_NODE_PORT by checking the
image documentation/changelog and, if needed, bump or change the image tag to
one that documents support for the new environment variable names.
| Assert.Equal(Int32.Parse(_sink.Data["database"]["epochNumber"].ToString()), | ||
| _electionsDoneMessage.ProposalNumber); | ||
|
|
There was a problem hiding this comment.
Bug: comparing epochNumber to proposalNumber
The test compares the sink’s database.epochNumber to ElectionsDone.ProposalNumber. These are not the same concept. The epoch number should be checked against the leader’s epoch number captured in mem1.
Apply this diff:
- Assert.Equal(Int32.Parse(_sink.Data["database"]["epochNumber"].ToString()),
- _electionsDoneMessage.ProposalNumber);
+ Assert.Equal(Int32.Parse(_sink.Data["database"]["epochNumber"].ToString()),
+ mem1.EpochNumber);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Assert.Equal(Int32.Parse(_sink.Data["database"]["epochNumber"].ToString()), | |
| _electionsDoneMessage.ProposalNumber); | |
| Assert.Equal( | |
| Int32.Parse(_sink.Data["database"]["epochNumber"].ToString()), | |
| mem1.EpochNumber); |
🤖 Prompt for AI Agents
In src/EventStore.Core.XUnit.Tests/Telemetry/TelemetryServiceTests.cs around
lines 199-201, the test incorrectly asserts that database.epochNumber equals
_electionsDoneMessage.ProposalNumber; instead assert that the sink’s
database.epochNumber equals the leader’s epoch number captured in mem1 (e.g.
mem1.Leader.EpochNumber or the variable that holds mem1’s leader epoch). Replace
the assertion to parse the sink value as before but compare it to mem1’s leader
epoch field rather than _electionsDoneMessage.ProposalNumber, keeping types
consistent (parse to int) and preserving existing test structure.
| public static ClusterVNodeOptions WithExternalTcpOn( | ||
| this ClusterVNodeOptions options, IPEndPoint endPoint) => | ||
| options with { Interface = options.Interface with { NodeIp = endPoint.Address, } }; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential endpoint conflation: WithExternalTcpOn() writes to NodeIp
WithExternalTcpOn sets Interface.NodeIp to the endpoint's address. If a caller configures different IPs for the node HTTP endpoint and the external TCP endpoint, this will overwrite the node IP set by WithNodeEndpointOn, likely producing an incorrect HTTP binding/advertise state. At minimum, the XML docs should clarify this behavior; ideally, the method should target a dedicated external-TCP field (if one exists) and avoid touching NodeIp.
Please verify how Interface maps to NodeInfo/transport bindings and whether a distinct ExternalTcp IP/port exists in Interface. To locate usages and assess impact:
If External TCP should not affect NodeIp, update this method to configure the dedicated external TCP fields (or deprecate it in favor of the node/replication APIs) and add a guard to prevent overwriting NodeIp when it has already been explicitly set by WithNodeEndpointOn.
🏁 Script executed:
#!/bin/bash
# Show all call sites for endpoint configuration helpers
rg -nPU -C3 '\.(WithNodeEndpointOn|WithExternalTcpOn|WithReplicationEndpointOn)\s*\(' --type=cs
# Inspect where NodeIp influences bindings/advertise
rg -nP -C3 --type=cs '\bNodeIp\b|\bNodePort\b|\bNodeHostAdvertiseAs\b|\bNodePortAdvertiseAs\b'
# Check resolved endpoints on the node
rg -nP -C3 --type=cs '\bHttpEndPoint\b|\bExternalSecureTcp\b|\bInternalSecureTcp\b'Length of output: 268647
Separate External TCP IP from NodeIp to Prevent Binding Conflation
The WithExternalTcpOn helper currently reuses Interface.NodeIp—the same field used by WithNodeEndpointOn for HTTP—leading to unintended overwrites when clients specify different IPs for HTTP vs. external TCP. We need a dedicated ExternalTcpIp (and optionally ExternalTcpPort) on the interface, update the helper and the node‐startup logic to use it, and preserve backwards compatibility by falling back to NodeIp when unset.
• Add new optional property to ClusterVNodeOptions.Interface
// in src/EventStore.Core/Configuration/ClusterVNodeOptions.cs (partial record)
[Description("The IP Address for the external TCP endpoint.")]
public IPAddress? ExternalTcpIp { get; init; } = null;• Update WithExternalTcpOn to set the new field rather than NodeIp
public static ClusterVNodeOptions WithExternalTcpOn(
this ClusterVNodeOptions options, IPEndPoint endPoint) =>
- options with { Interface = options.Interface with { NodeIp = endPoint.Address, } };
+ options with { Interface = options.Interface with { ExternalTcpIp = endPoint.Address, } };• Modify node startup in ClusterVNode.cs to use ExternalTcpIp when present
// before constructing external TCP endpoints
- var extTcp = disableExternalTcpTls && enableExternalTcp
- ? new IPEndPoint(options.Interface.NodeIp, nodeTcpOptions.NodeTcpPort)
+ var extIp = options.Interface.ExternalTcpIp ?? options.Interface.NodeIp;
+ var extTcp = disableExternalTcpTls && enableExternalTcp
+ ? new IPEndPoint(extIp, nodeTcpOptions.NodeTcpPort)
: null;
var extSecTcp = !disableExternalTcpTls && enableExternalTcp
- ? new IPEndPoint(options.Interface.NodeIp, nodeTcpOptions.NodeTcpPort)
+ ? new IPEndPoint(extIp, nodeTcpOptions.NodeTcpPort)
: null;• Add XML doc comments to clarify new behavior on both the extension method and the option field
• Update any failing tests or add coverage to ensure fallback behaviour remains unchanged when ExternalTcpIp is unset
🤖 Prompt for AI Agents
In src/EventStore.Core/Configuration/ClusterVNodeOptionsExtensions.cs around
lines 105-108, the WithExternalTcpOn helper currently writes into
Interface.NodeIp and thus can overwrite HTTP node IPs; add a new optional
IPAddress? ExternalTcpIp (and optionally ExternalTcpPort) to the
ClusterVNodeOptions.Interface record (with XML doc comments and Description
attribute) in src/EventStore.Core/Configuration/ClusterVNodeOptions.cs, change
WithExternalTcpOn to set Interface.ExternalTcpIp (and port if added) instead of
NodeIp, update ClusterVNode.cs node-startup to prefer Interface.ExternalTcpIp
when present and fall back to Interface.NodeIp when ExternalTcpIp is null, add
XML docs to the extension and option field, and update or add tests to verify
that ExternalTcpIp takes precedence and that behavior falls back to NodeIp when
ExternalTcpIp is unset.
| var payload = JsonSerializer.SerializeToNode( | ||
| evt.Data.ToDictionary(kvp => LowerFirstLetter(kvp.Key), kvp => kvp.Value)); | ||
| message.Envelope.ReplyWith(new TelemetryMessage.Response(LowerFirstLetter(evt.Source), payload)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid ToDictionary collisions and exceptions on key normalization
evt.Data.ToDictionary(...) will throw if two keys differ only by the first letter’s case (e.g., "Cloud" and "cloud") after LowerFirstLetter. Prefer last-write-wins into a normal Dictionary to eliminate the exception and make behavior predictable.
Apply:
- var payload = JsonSerializer.SerializeToNode(
- evt.Data.ToDictionary(kvp => LowerFirstLetter(kvp.Key), kvp => kvp.Value));
- message.Envelope.ReplyWith(new TelemetryMessage.Response(LowerFirstLetter(evt.Source), payload));
+ var normalized = new Dictionary<string, object?>(StringComparer.Ordinal);
+ foreach (var kvp in evt.Data)
+ normalized[LowerFirstLetter(kvp.Key)] = kvp.Value;
+ var payload = JsonSerializer.SerializeToNode(normalized);
+ message.Envelope.ReplyWith(new TelemetryMessage.Response(LowerFirstLetter(evt.Source), payload));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var payload = JsonSerializer.SerializeToNode( | |
| evt.Data.ToDictionary(kvp => LowerFirstLetter(kvp.Key), kvp => kvp.Value)); | |
| message.Envelope.ReplyWith(new TelemetryMessage.Response(LowerFirstLetter(evt.Source), payload)); | |
| } | |
| var normalized = new Dictionary<string, object?>(StringComparer.Ordinal); | |
| foreach (var kvp in evt.Data) | |
| normalized[LowerFirstLetter(kvp.Key)] = kvp.Value; | |
| var payload = JsonSerializer.SerializeToNode(normalized); | |
| message.Envelope.ReplyWith(new TelemetryMessage.Response(LowerFirstLetter(evt.Source), payload)); |
🤖 Prompt for AI Agents
In src/EventStore.Core/Telemetry/TelemetryService.cs around lines 256 to 259,
replace the ToDictionary call (which throws on key collisions when keys differ
only by first-letter case) with building a regular Dictionary<string, object>
and assign entries using the normalized key (LowerFirstLetter) so later values
overwrite earlier ones (last-write-wins); then pass that Dictionary to
JsonSerializer.SerializeToNode and keep the rest of the message creation
unchanged.
| private static string LowerFirstLetter(string x) | ||
| { | ||
| if (string.IsNullOrEmpty(x) || char.IsLower(x[0])) | ||
| return x; | ||
|
|
||
| return $"{char.ToLower(x[0])}{x[1..]}"; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use culture-invariant lowercasing for stable key normalization
char.ToLower is culture-sensitive; use ToLowerInvariant to avoid locale-dependent output (e.g., Turkish-i).
- private static string LowerFirstLetter(string x)
+ private static string LowerFirstLetter(string x)
{
if (string.IsNullOrEmpty(x) || char.IsLower(x[0]))
return x;
- return $"{char.ToLower(x[0])}{x[1..]}";
+ return $"{char.ToLowerInvariant(x[0])}{x[1..]}";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static string LowerFirstLetter(string x) | |
| { | |
| if (string.IsNullOrEmpty(x) || char.IsLower(x[0])) | |
| return x; | |
| return $"{char.ToLower(x[0])}{x[1..]}"; | |
| } | |
| private static string LowerFirstLetter(string x) | |
| { | |
| if (string.IsNullOrEmpty(x) || char.IsLower(x[0])) | |
| return x; | |
| return $"{char.ToLowerInvariant(x[0])}{x[1..]}"; | |
| } |
🤖 Prompt for AI Agents
In src/EventStore.Core/Telemetry/TelemetryService.cs around lines 279 to 285,
the helper LowerFirstLetter uses char.ToLower which is culture-sensitive;
replace the call with char.ToLowerInvariant(x[0]) so the first character is
lowercased in a culture-invariant way (keeping the rest of the logic intact) to
ensure stable key normalization across locales.
12a0c46 to
0a22075
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/EventStore.Core.XUnit.Tests/Configuration/ClusterVNodeOptionsTests.cs (1)
101-111: Avoid leaking process-wide env vars from testsEnvironment.SetEnvironmentVariable("EVENTSTORE__CLUSTER_SIZE", "3") is not cleaned up and may affect subsequent tests in the process. Wrap in try/finally and clear the variable.
- Environment.SetEnvironmentVariable("EVENTSTORE__CLUSTER_SIZE", "3"); - - var configuration = new ConfigurationBuilder() + const string varName = "EVENTSTORE__CLUSTER_SIZE"; + Environment.SetEnvironmentVariable(varName, "3"); + try + { + var configuration = new ConfigurationBuilder() .AddEnvironmentVariables() .AddEventStoreEnvironmentVariables() .Build(); - var options = ClusterVNodeOptions.FromConfiguration(configuration); - options.Unknown.Options.Should().BeEmpty(); + var options = ClusterVNodeOptions.FromConfiguration(configuration); + options.Unknown.Options.Should().BeEmpty(); + } + finally + { + Environment.SetEnvironmentVariable(varName, null); + }
♻️ Duplicate comments (4)
src/EventStore.Core.XUnit.Tests/Telemetry/TelemetryServiceTests.cs (1)
199-200: Bug: comparing epochNumber to proposalNumberThe test compares the sink's database.epochNumber to ElectionsDone.ProposalNumber. These are not the same concept. The epoch number should be checked against the leader's epoch number captured in mem1.
Apply this diff:
- Assert.Equal(Int32.Parse(_sink.Data["database"]["epochNumber"].ToString()), - _electionsDoneMessage.ProposalNumber); + Assert.Equal(Int32.Parse(_sink.Data["database"]["epochNumber"].ToString()), + mem1.EpochNumber);src/EventStore.Core/Configuration/ClusterVNodeOptionsExtensions.cs (3)
105-108: WithExternalTcpOn overwrites NodeIp; deprecate to prevent endpoint conflationThis helper sets Interface.NodeIp and can undo a previous WithNodeEndpointOn call, conflating external TCP with HTTP bindings. Either repoint it to a dedicated ExternalTcpIp (and optional port), or mark it obsolete in favor of WithNodeEndpointOn/WithReplicationEndpointOn.
Minimal, non-breaking mitigation via Obsolete attribute:
/// <summary> /// Sets the external tcp endpoint to the specified value /// </summary> + [Obsolete("Use WithNodeEndpointOn (HTTP) and WithReplicationEndpointOn (internal TCP) instead. This method overwrites NodeIp.")] public static ClusterVNodeOptions WithExternalTcpOn( this ClusterVNodeOptions options, IPEndPoint endPoint) => options with { Interface = options.Interface with { NodeIp = endPoint.Address, } };Preferred long-term fix: introduce Interface.ExternalTcpIp (and port), update this helper to set it, and alter startup to use ExternalTcpIp ?? NodeIp. Happy to draft the full patch if you want.
176-216: Guard against partial store configuration when loading node certificate and improve diagnosticsIf CertificateStoreLocation is set but CertificateStoreName is empty, GetCertificateStoreName will throw a generic parsing error. Fail fast with a clear configuration message and log the chosen store.
- if (!string.IsNullOrWhiteSpace(options.CertificateStore.CertificateStoreLocation)) - { - var location = - CertificateUtils.GetCertificateStoreLocation(options.CertificateStore.CertificateStoreLocation); - var name = CertificateUtils.GetCertificateStoreName(options.CertificateStore.CertificateStoreName); - return (CertificateUtils.LoadFromStore(location, name, options.CertificateStore.CertificateSubjectName, - options.CertificateStore.CertificateThumbprint), null); - } + if (!string.IsNullOrWhiteSpace(options.CertificateStore.CertificateStoreLocation)) + { + if (string.IsNullOrWhiteSpace(options.CertificateStore.CertificateStoreName)) + throw new InvalidConfigurationException( + "CertificateStoreName must be specified when CertificateStoreLocation is set."); + + var location = + CertificateUtils.GetCertificateStoreLocation(options.CertificateStore.CertificateStoreLocation); + var name = CertificateUtils.GetCertificateStoreName(options.CertificateStore.CertificateStoreName); + Log.Information("Loading node certificate from store: {location}/{name} subject={subject} thumbprint={thumbprint}", + location, name, options.CertificateStore.CertificateSubjectName, options.CertificateStore.CertificateThumbprint); + return (CertificateUtils.LoadFromStore(location, name, options.CertificateStore.CertificateSubjectName, + options.CertificateStore.CertificateThumbprint), null); + } - if (!string.IsNullOrWhiteSpace(options.CertificateStore.CertificateStoreName)) - { - var name = CertificateUtils.GetCertificateStoreName(options.CertificateStore.CertificateStoreName); - return ( - CertificateUtils.LoadFromStore(name, options.CertificateStore.CertificateSubjectName, - options.CertificateStore.CertificateThumbprint), null); - } + if (!string.IsNullOrWhiteSpace(options.CertificateStore.CertificateStoreName)) + { + var name = CertificateUtils.GetCertificateStoreName(options.CertificateStore.CertificateStoreName); + Log.Information("Loading node certificate from default store location: {name} subject={subject} thumbprint={thumbprint}", + name, options.CertificateStore.CertificateSubjectName, options.CertificateStore.CertificateThumbprint); + return ( + CertificateUtils.LoadFromStore(name, options.CertificateStore.CertificateSubjectName, + options.CertificateStore.CertificateThumbprint), null); + }
227-273: Apply the same partial-config guard to trusted rootsMatch the guard/logging pattern for TrustedRootCertificateStoreLocation/Name to avoid obscure exceptions and improve operability.
- if (!string.IsNullOrWhiteSpace(options.CertificateStore.TrustedRootCertificateStoreLocation)) - { - var location = - CertificateUtils.GetCertificateStoreLocation(options.CertificateStore - .TrustedRootCertificateStoreLocation); - var name = CertificateUtils.GetCertificateStoreName(options.CertificateStore - .TrustedRootCertificateStoreName); - trustedRootCerts.Add(CertificateUtils.LoadFromStore(location, name, - options.CertificateStore.TrustedRootCertificateSubjectName, - options.CertificateStore.TrustedRootCertificateThumbprint)); - return trustedRootCerts; - } + if (!string.IsNullOrWhiteSpace(options.CertificateStore.TrustedRootCertificateStoreLocation)) + { + if (string.IsNullOrWhiteSpace(options.CertificateStore.TrustedRootCertificateStoreName)) + throw new InvalidConfigurationException( + "TrustedRootCertificateStoreName must be specified when TrustedRootCertificateStoreLocation is set."); + + var location = + CertificateUtils.GetCertificateStoreLocation(options.CertificateStore + .TrustedRootCertificateStoreLocation); + var name = CertificateUtils.GetCertificateStoreName(options.CertificateStore + .TrustedRootCertificateStoreName); + Log.Information("Loading trusted roots from store: {location}/{name} subject={subject} thumbprint={thumbprint}", + location, name, options.CertificateStore.TrustedRootCertificateSubjectName, + options.CertificateStore.TrustedRootCertificateThumbprint); + trustedRootCerts.Add(CertificateUtils.LoadFromStore(location, name, + options.CertificateStore.TrustedRootCertificateSubjectName, + options.CertificateStore.TrustedRootCertificateThumbprint)); + return trustedRootCerts; + } - if (!string.IsNullOrWhiteSpace(options.CertificateStore.TrustedRootCertificateStoreName)) - { - var name = CertificateUtils.GetCertificateStoreName(options.CertificateStore - .TrustedRootCertificateStoreName); - trustedRootCerts.Add(CertificateUtils.LoadFromStore(name, - options.CertificateStore.TrustedRootCertificateSubjectName, - options.CertificateStore.TrustedRootCertificateThumbprint)); - return trustedRootCerts; - } + if (!string.IsNullOrWhiteSpace(options.CertificateStore.TrustedRootCertificateStoreName)) + { + var name = CertificateUtils.GetCertificateStoreName(options.CertificateStore + .TrustedRootCertificateStoreName); + Log.Information("Loading trusted roots from default store location: {name} subject={subject} thumbprint={thumbprint}", + name, options.CertificateStore.TrustedRootCertificateSubjectName, + options.CertificateStore.TrustedRootCertificateThumbprint); + trustedRootCerts.Add(CertificateUtils.LoadFromStore(name, + options.CertificateStore.TrustedRootCertificateSubjectName, + options.CertificateStore.TrustedRootCertificateThumbprint)); + return trustedRootCerts; + }
🧹 Nitpick comments (11)
src/EventStore.Core/Configuration/ClusterVNodeOptions.cs (7)
32-33: Constructor side-effect is acceptable but consider documenting ConfigureFlush behaviorCalling FileStreamExtensions.ConfigureFlush(Database.UnsafeDisableFlushToDisk) in the constructor is fine, but it globally affects flush behavior each time an instance is created. A short XML doc comment clarifying this side effect will prevent surprises during tests or tooling that instantiate options frequently.
256-264: Trusted roots default path: consider explicit null for “unset”TrustedRootCertificatesPath defaulting to Locations.DefaultTrustedRootCertificateDirectory is convenient, but it makes it harder to detect “unset vs. defaulted”. If you need to distinguish those cases later, consider defaulting to null and setting the default in binding. Not blocking.
376-378: Use shared constant for HashCollisionReadLimitKeep one source of truth by referencing Opts.HashCollisionReadLimitDefault.
- public int HashCollisionReadLimit { get; init; } = 100; + public int HashCollisionReadLimit { get; init; } = Opts.HashCollisionReadLimitDefault;
457-466: WriteStatsToDb accessor mutates StatsStorage in init; acceptable but surprisingThe init accessor side-effectfully sets StatsStorage. It’s valid, but unexpected for an init-only property. A short doc comment or moving the coupling to FromConfiguration would make intent clearer. Not blocking.
563-568: Reuse Opts defaults for queue/bytes thresholdsAvoid duplicated literals; use the constants already defined in Opts to prevent drift.
- public int ConnectionPendingSendBytesThreshold { get; init; } = 10 * 1_024 * 1_024; + public int ConnectionPendingSendBytesThreshold { get; init; } = Opts.ConnectionPendingSendBytesThresholdDefault; … - public int ConnectionQueueSizeThreshold { get; init; } = 50_000; + public int ConnectionQueueSizeThreshold { get; init; } = Opts.ConnectionQueueSizeThresholdDefault;
592-609: Projection defaults: consider centralizing in OptsProjectionsQueryExpiry and FaultOutOfOrderProjections have counterparts in Opts. Use the constants to keep defaults consistent across code paths and docs.
- public int ProjectionsQueryExpiry { get; init; } = 5; + public int ProjectionsQueryExpiry { get; init; } = Opts.ProjectionsQueryExpiryDefault; - public bool FaultOutOfOrderProjections { get; init; } = false; + public bool FaultOutOfOrderProjections { get; init; } = Opts.FaultOutOfOrderProjectionsDefault;
620-684: Unknown options detection: make top-level filtering case-insensitiveWhen filtering out nested unknown keys, StartsWith uses culture-sensitive defaults and the HashSet for sections is case-sensitive. Use OrdinalIgnoreCase consistently to avoid missing sections where casing differs across providers.
- static HashSet<string> FindUnknownSections(IEnumerable<string> keys) + static HashSet<string> FindUnknownSections(IEnumerable<string> keys) { - var hashSet = new HashSet<string>(); + var hashSet = new HashSet<string>(StringComparer.OrdinalIgnoreCase); foreach (var key in keys.Where(key => key.Split(":").Length > 2)) hashSet.Add(key[..key.LastIndexOf(':')]); return hashSet; } … - return unknownKeys - .Where(key => !unknownSections.Any(key.StartsWith)); + return unknownKeys + .Where(key => !unknownSections.Any(section => key.StartsWith(section, StringComparison.OrdinalIgnoreCase)));src/EventStore.Core/Configuration/ClusterVNodeOptionsExtensions.cs (1)
41-44: Avoid constructing a new ClusterVNodeOptions just to read the default DB pathnew ClusterVNodeOptions() has side effects (see ConfigureFlush). Prefer the constant source for the default path.
public static ClusterVNodeOptions RunInMemory(this ClusterVNodeOptions options) => options with { - Database = options.Database with { MemDb = true, Db = new ClusterVNodeOptions().Database.Db } + Database = options.Database with { MemDb = true, Db = Locations.DefaultDataDirectory } };src/EventStore.Core/Configuration/ClusterVNodeOptions.Framework.cs (1)
206-215: DefaultValue formatting: consider consistent casing across platformsBooleans are lower-case on non-Windows and default casing on Windows. For consistent CLI/docs output, consider normalizing to lower-case across platforms. Not blocking.
src/EventStore.Core.XUnit.Tests/Configuration/ClusterVNodeOptionsTests.cs (2)
210-220: NodeIp binding via env var: LGTMGood to pin explicit IPv4. Consider an IPv6 case in a follow-up to validate converter behavior with bracketed addresses.
153-171: GossipSeed via single env var: consider using AddEventStoreEnvironmentVariables onlyYou already use AddEventStoreEnvironmentVariables to inject a scoped var; sticking to that avoids touching real process env and keeps tests hermetic. Not blocking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (17)
docker-compose.yml(3 hunks)samples/server/docker-compose.yaml(1 hunks)shared.env(1 hunks)src/EventStore.Core.Tests/Helpers/MiniClusterNode.cs(0 hunks)src/EventStore.Core.Tests/Helpers/MiniNode.cs(2 hunks)src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_cluster_node_and_custom_settings.cs(4 hunks)src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_default_settings.cs(0 hunks)src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_secure_tcp.cs(3 hunks)src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_single_node_and_custom_settings.cs(2 hunks)src/EventStore.Core.XUnit.Tests/Configuration/ClusterVNodeOptionsTests.cs(3 hunks)src/EventStore.Core.XUnit.Tests/Telemetry/TelemetryServiceTests.cs(9 hunks)src/EventStore.Core/Authentication/PassthroughAuthentication/PassthroughAuthenticationProvider.cs(1 hunks)src/EventStore.Core/ClusterVNode.cs(1 hunks)src/EventStore.Core/Configuration/ClusterVNodeOptions.Framework.cs(1 hunks)src/EventStore.Core/Configuration/ClusterVNodeOptions.cs(1 hunks)src/EventStore.Core/Configuration/ClusterVNodeOptionsExtensions.cs(1 hunks)src/EventStore.Core/Telemetry/TelemetryService.cs(7 hunks)
💤 Files with no reviewable changes (2)
- src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_default_settings.cs
- src/EventStore.Core.Tests/Helpers/MiniClusterNode.cs
🚧 Files skipped from review as they are similar to previous changes (10)
- src/EventStore.Core/Authentication/PassthroughAuthentication/PassthroughAuthenticationProvider.cs
- samples/server/docker-compose.yaml
- src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_secure_tcp.cs
- docker-compose.yml
- src/EventStore.Core.Tests/Helpers/MiniNode.cs
- src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_single_node_and_custom_settings.cs
- src/EventStore.Core/Telemetry/TelemetryService.cs
- src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_cluster_node_and_custom_settings.cs
- src/EventStore.Core/ClusterVNode.cs
- shared.env
🧰 Additional context used
🧬 Code graph analysis (2)
src/EventStore.Core/Configuration/ClusterVNodeOptions.cs (4)
src/EventStore.Core/Configuration/ClusterVNodeOptionsExtensions.cs (16)
ClusterVNodeOptions(16-19)ClusterVNodeOptions(21-23)ClusterVNodeOptions(25-34)ClusterVNodeOptions(41-44)ClusterVNodeOptions(52-55)ClusterVNodeOptions(62-67)ClusterVNodeOptions(76-82)ClusterVNodeOptions(90-97)ClusterVNodeOptions(105-107)ClusterVNodeOptions(115-120)ClusterVNodeOptions(128-130)ClusterVNodeOptions(138-140)ClusterVNodeOptions(148-157)ClusterVNodeOptions(164-171)X509Certificate2(178-216)X509Certificate2Collection(227-273)src/EventStore.Core/Configuration/TypeConverters.cs (4)
EndPoint(19-31)GossipSeedConverter(34-57)GossipEndPointConverter(10-32)IPAddressConverter(59-67)src/EventStore.Core/Util/Opts.cs (1)
Opts(7-33)src/EventStore.Core/Configuration/Sources/EventStoreConfigurationKeys.cs (3)
EventStoreConfigurationKeys(13-94)EventStoreConfigurationKeys(26-40)Normalize(43-72)
src/EventStore.Core/Configuration/ClusterVNodeOptionsExtensions.cs (4)
src/EventStore.Core/Configuration/ClusterVNodeOptions.cs (2)
ClusterVNodeOptions(32-32)ClusterVNodeOptions(60-94)src/EventStore.Common/Utils/EndpointExtensions.cs (2)
GetHost(23-31)GetPort(43-50)src/EventStore.Core/Certificates/CertificateUtils.cs (1)
CertificateUtils(13-315)src/EventStore.Common/Utils/StringExtensions.cs (1)
IsNotEmptyString(14-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build / ubuntu-24.04
- GitHub Check: Build / noble
- GitHub Check: Build / ubuntu-24.04-arm
- GitHub Check: Build / bookworm-slim
- GitHub Check: Docker Compose Smoke Test
🔇 Additional comments (22)
src/EventStore.Core.XUnit.Tests/Telemetry/TelemetryServiceTests.cs (10)
2-2: LGTM!The additional imports for
System.Collections.GenericandMicrosoft.Extensions.Configurationare appropriate for the new configuration-based telemetry setup.Also applies to: 18-18
52-54: LGTM!The configuration setup correctly provides the CloudIdentifier via in-memory configuration, which aligns with the updated TelemetryService constructor that now accepts IConfiguration.
76-95: LGTM!The use of named parameters in the
MemberInfo.ForVNodecall improves code readability and maintainability.
122-122: LGTM!The inline JsonObject initialization is more concise and readable.
137-143: LGTM!The test correctly verifies the updated telemetry payload structure with the "fakeComponent" section containing "baz": "qux".
147-153: LGTM!The test correctly verifies that the CloudIdentifier from configuration is included in the telemetry data.
185-185: LGTM!Consistent use of inline JsonObject initialization improves readability.
203-204: LGTM!These assertions correctly compare the database epoch number with the leader's epoch number from the respective messages.
Also applies to: 207-208
213-213: LGTM!The updated class name "FakeComponent" (instead of "fakeComponent") follows proper C# naming conventions for class names.
217-220: LGTM!The updated telemetry data with "enabled" and "Baz" keys aligns with the test expectations and the new payload structure.
src/EventStore.Core/Configuration/ClusterVNodeOptions.cs (6)
96-106: Environment-only defaults for credentials look goodDefaultUserOptions correctly marks passwords as Sensitive and EnvironmentOnly. No issues.
212-230: Auth defaults consistent with prior behaviorAuthorizationType/AuthenticationType default to "internal" and deprecation note on first-level HTTP auth is clear. Looks good.
243-251: Add private-key password support: nice improvementSupport for CertificatePrivateKeyPassword (PKCS #8 encrypted keys) closes a common ops gap. LGTM.
350-351: QuorumSize calculation is correctInteger math covers both single-node and multi-node clusters as expected.
481-496: Good additions for tunables (existence filter, scavenge backend sizing)Defaults wired to Opts constants provide clarity and a single source of truth.
512-517: GrpcOptions.FromConfiguration may not read from the intended sectionFromConfiguration uses raw property names against the provided IConfiguration rather than a section scoped to Grpc. If callers pass the root “EventStore” section, values won’t resolve from “EventStore:Grpc:...”. Please verify the call sites and consider binding from configuration.GetSection("Grpc").
Would you like a repo scan to find usages and confirm the section semantics?src/EventStore.Core/Configuration/ClusterVNodeOptionsExtensions.cs (2)
115-121: Replication endpoint helper reads clearly and avoids conflationSetting ReplicationIp and ReplicationPort together is the right call. LGTM.
128-131: HTTP/node endpoint helper: LGTMExplicitly configuring NodeIp and NodePort keeps semantics clear.
src/EventStore.Core/Configuration/ClusterVNodeOptions.Framework.cs (3)
20-21: GetComponentName composition is clearComponent naming based on NodeIp/NodePort reads well.
96-112: Array-style option loading: pragmatic and effectiveHandling indexed arrays when the parent key lacks a scalar value addresses GossipSeed cleanly. The comment reflects current scope; the logic is generic enough for other array options if added later. LGTM.
129-141: Provider source display name resolution looks solidRecursive handling of SectionProvider helps produce accurate provenance.
src/EventStore.Core.XUnit.Tests/Configuration/ClusterVNodeOptionsTests.cs (1)
173-190: Array-based GossipSeed test: LGTMCoverage for indexed configuration loading is valuable and matches the implemented behavior.
Changed docker samples (environment variables) Changed Telemetry (allow extra config data, normalize keys to lowercase) Fix support for gossip seeds provided as an array Calculate in linux disk stats correctly for mount point instead of root Signed-off-by: JOSE FUXA <[email protected]>
j-fuxa
force-pushed
the
docker_telemetry
branch
from
0a22075 to
8eced24
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
src/EventStore.Core.XUnit.Tests/Telemetry/TelemetryServiceTests.cs (1)
199-201: Bug: comparing epochNumber to proposalNumber (duplicate of earlier review)database.epochNumber should be compared to the leader’s epoch (mem1.EpochNumber), not ElectionsDone.ProposalNumber.
- Assert.Equal(Int32.Parse(_sink.Data["database"]["epochNumber"].ToString()), - _electionsDoneMessage.ProposalNumber); + Assert.Equal(Int32.Parse(_sink.Data["database"]["epochNumber"].ToString()), + mem1.EpochNumber);src/EventStore.Core/Configuration/ClusterVNodeOptionsExtensions.cs (3)
105-108: External TCP helper overwrites Node HTTP IP and ignores portSetting Interface.NodeIp inside WithExternalTcpOn couples the external TCP binding to the HTTP/node binding and drops the provided port. This will overwrite a previously set node HTTP IP (e.g., via WithNodeEndpointOn) and can yield incorrect HTTP binds/advertise results; the port is also ignored.
Apply a dedicated ExternalTcp* option instead of Node*. Minimal fix in this file:
public static ClusterVNodeOptions WithExternalTcpOn( this ClusterVNodeOptions options, IPEndPoint endPoint) => - options with { Interface = options.Interface with { NodeIp = endPoint.Address, } }; + options with { + Interface = options.Interface with { + ExternalTcpIp = endPoint.Address, + ExternalTcpPort = endPoint.Port + } + };Outside this file, add the corresponding properties and prefer them when constructing external TCP endpoints; fall back to NodeIp/NodeTcpPort if unset. I can provide a full patch if helpful.
Run this quick scan to find call sites that combine HTTP and External TCP helpers (risk of overwrites):
#!/bin/bash rg -nPU -C2 --type=cs '(?s)\.WithNodeEndpointOn\(.+?\).*?\.WithExternalTcpOn\(' rg -nPU -C2 --type=cs '(?s)\.WithExternalTcpOn\(.+?\).*?\.WithNodeEndpointOn\(' rg -nP --type=cs '\bNodeIp\b|\bExternalTcpIp\b|\bExternalTcpPort\b|\bNodePort\b'
188-204: Guard against partial store config and log chosen store when loading node certificateIf CertificateStoreLocation is set but CertificateStoreName is empty/whitespace, GetCertificateStoreName will throw with a parsing error. Fail fast with a clear configuration message and log the selected store for operability.
if (!string.IsNullOrWhiteSpace(options.CertificateStore.CertificateStoreLocation)) { + if (string.IsNullOrWhiteSpace(options.CertificateStore.CertificateStoreName)) + throw new InvalidConfigurationException( + "CertificateStoreName must be specified when CertificateStoreLocation is set."); + var location = CertificateUtils.GetCertificateStoreLocation(options.CertificateStore.CertificateStoreLocation); var name = CertificateUtils.GetCertificateStoreName(options.CertificateStore.CertificateStoreName); + Log.Information("Loading node certificate from store: {location}/{name} subject={subject} thumbprint={thumbprint}", + location, name, options.CertificateStore.CertificateSubjectName, options.CertificateStore.CertificateThumbprint); return (CertificateUtils.LoadFromStore(location, name, options.CertificateStore.CertificateSubjectName, options.CertificateStore.CertificateThumbprint), null); } if (!string.IsNullOrWhiteSpace(options.CertificateStore.CertificateStoreName)) { var name = CertificateUtils.GetCertificateStoreName(options.CertificateStore.CertificateStoreName); + Log.Information("Loading node certificate from default store location: {name} subject={subject} thumbprint={thumbprint}", + name, options.CertificateStore.CertificateSubjectName, options.CertificateStore.CertificateThumbprint); return ( CertificateUtils.LoadFromStore(name, options.CertificateStore.CertificateSubjectName, options.CertificateStore.CertificateThumbprint), null); }
232-253: Same guard/logging for trusted roots loaded from storeMirror the node-certificate fix to avoid ambiguous exceptions and improve diagnostics.
if (!string.IsNullOrWhiteSpace(options.CertificateStore.TrustedRootCertificateStoreLocation)) { + if (string.IsNullOrWhiteSpace(options.CertificateStore.TrustedRootCertificateStoreName)) + throw new InvalidConfigurationException( + "TrustedRootCertificateStoreName must be specified when TrustedRootCertificateStoreLocation is set."); + var location = CertificateUtils.GetCertificateStoreLocation(options.CertificateStore .TrustedRootCertificateStoreLocation); var name = CertificateUtils.GetCertificateStoreName(options.CertificateStore .TrustedRootCertificateStoreName); + Log.Information("Loading trusted roots from store: {location}/{name} subject={subject} thumbprint={thumbprint}", + location, name, options.CertificateStore.TrustedRootCertificateSubjectName, + options.CertificateStore.TrustedRootCertificateThumbprint); trustedRootCerts.Add(CertificateUtils.LoadFromStore(location, name, options.CertificateStore.TrustedRootCertificateSubjectName, options.CertificateStore.TrustedRootCertificateThumbprint)); return trustedRootCerts; } if (!string.IsNullOrWhiteSpace(options.CertificateStore.TrustedRootCertificateStoreName)) { var name = CertificateUtils.GetCertificateStoreName(options.CertificateStore .TrustedRootCertificateStoreName); + Log.Information("Loading trusted roots from default store location: {name} subject={subject} thumbprint={thumbprint}", + name, options.CertificateStore.TrustedRootCertificateSubjectName, + options.CertificateStore.TrustedRootCertificateThumbprint); trustedRootCerts.Add(CertificateUtils.LoadFromStore(name, options.CertificateStore.TrustedRootCertificateSubjectName, options.CertificateStore.TrustedRootCertificateThumbprint)); return trustedRootCerts; }
🧹 Nitpick comments (20)
src/EventStore.SystemRuntime/Diagnostics/DriveStats.cs (5)
7-7: Make DriveStats static (no instance state).The type was changed to non-static but only exposes static members. Marking it static clarifies intent and avoids accidental instantiation.
-public class DriveStats +public static class DriveStats
9-9: Avoid exposing a public logger in the API surface.Publishing a public static ILogger leaks implementation details and encourages external code to couple to your logging context. Prefer internal/private.
- public static readonly ILogger Log = Serilog.Log.ForContext<DriveStats>(); + internal static readonly ILogger Log = Serilog.Log.ForContext<DriveStats>();
17-22: Use OS-appropriate, ordinal comparisons for file-system paths.InvariantCultureIgnoreCase can misbehave with certain locales and is unnecessary for path matching. Use ordinal ignore-case on Windows and ordinal on Unix-like systems.
- var diskName = ""; + var diskName = ""; + var comparison = Path.DirectorySeparatorChar == '\\' + ? StringComparison.OrdinalIgnoreCase + : StringComparison.Ordinal; foreach (var candidate in DriveInfo.GetDrives()) { - if (target.StartsWith(candidate.Name, StringComparison.InvariantCultureIgnoreCase) && - candidate.Name.StartsWith(diskName, StringComparison.InvariantCultureIgnoreCase)) + if (target.StartsWith(candidate.Name, comparison) && + candidate.Name.StartsWith(diskName, comparison)) { diskName = candidate.Name; } }
15-30: Handle non-ready drives explicitly (reduces exception noise).Accessing TotalSize/AvailableFreeSpace on a non-ready drive can throw. You already catch exceptions; short-circuiting improves signal and avoids stack traces in common transient states.
var info = new DriveInfo(Path.GetFullPath(path)); var target = info.Name; var diskName = ""; + if (!info.IsReady) + { + Log.Warning("Drive {Drive} not ready when retrieving stats for {Path}", info.Name, path); + return new DriveData("Unknown", 0, 0); + } + foreach (var candidate in DriveInfo.GetDrives())
50-55: Prefer expression-bodied computed properties to avoid redundant backing fields.Keeps DriveData minimal and derives values from the primaries without extra storage.
- public long UsedBytes { get; } = TotalBytes - AvailableBytes; + public long UsedBytes => TotalBytes - AvailableBytes; @@ - public int Usage { get; } = (int)(TotalBytes != 0 ? (TotalBytes - AvailableBytes) * 100 / TotalBytes : 0); + public int Usage => TotalBytes != 0 + ? (int)((TotalBytes - AvailableBytes) * 100 / TotalBytes) + : 0;src/EventStore.Core.XUnit.Tests/Telemetry/TelemetryServiceTests.cs (4)
137-144: Make JSON assertions resilient (avoid pretty-print brittleness for fakeComponent)Comparing JsonObject.ToString() to a hand-indented multi-line literal is brittle wrt whitespace/ordering. Assert on fields or compute the expected JSON using JsonObject to keep formatting consistent.
Suggested change:
- Assert.NotNull(_sink.Data["fakeComponent"]); - Assert.Equal(""" - { - "baz": "qux" - } - """, - _sink.Data["fakeComponent"].ToString()); + Assert.NotNull(_sink.Data["fakeComponent"]); + Assert.Equal("qux", _sink.Data["fakeComponent"]!["baz"]!.ToString());
147-154: Same brittleness for telemetry.cloudIdentifierAssert the value directly to avoid formatting issues.
- Assert.NotNull(_sink.Data["telemetry"]); - Assert.Equal(""" - { - "cloudIdentifier": "abc" - } - """, - _sink.Data["telemetry"].ToString()); + Assert.NotNull(_sink.Data["telemetry"]); + Assert.Equal("abc", _sink.Data["telemetry"]!["cloudIdentifier"]!.ToString());
145-145: Swap expected/actual for clearer assertion messagesxUnit’s Assert.Equal takes expected first. Swap for readability in failures.
- Assert.Equal(_sink.Data["environment"]!["os"]!.ToString(), RuntimeInformation.OSDescription); + Assert.Equal(RuntimeInformation.OSDescription, _sink.Data["environment"]!["os"]!.ToString());
38-41: Avoid disposing a Task; prefer synchronous wait pattern in ctorusing (...) on a Task is unnecessary. If you can’t await in a ctor, prefer GetAwaiter().GetResult() or move open into InitializeAsync().
- using (var task = _db.Open(false).AsTask()) - { - task.Wait(); - } + _db.Open(false).GetAwaiter().GetResult();If desired, I can move TFChunkDb.Open into InitializeAsync to avoid sync-over-async in the ctor.
src/EventStore.Core/Configuration/ClusterVNodeOptions.cs (9)
64-69: Register TypeConverters only once to avoid repeated global mutationTypeDescriptor.AddAttributes mutates global state and is called on every FromConfiguration invocation. Guard it so registration happens once.
Within the record, add a static flag:
+private static volatile bool s_typeConvertersRegistered;Guard registrations:
- TypeDescriptor.AddAttributes(typeof(EndPoint[]), new TypeConverterAttribute(typeof(GossipSeedConverter))); - TypeDescriptor.AddAttributes(typeof(EndPoint), new TypeConverterAttribute(typeof(GossipEndPointConverter))); - TypeDescriptor.AddAttributes(typeof(IPAddress), new TypeConverterAttribute(typeof(IPAddressConverter))); + if (!s_typeConvertersRegistered) { + TypeDescriptor.AddAttributes(typeof(EndPoint[]), new TypeConverterAttribute(typeof(GossipSeedConverter))); + TypeDescriptor.AddAttributes(typeof(EndPoint), new TypeConverterAttribute(typeof(GossipEndPointConverter))); + TypeDescriptor.AddAttributes(typeof(IPAddress), new TypeConverterAttribute(typeof(IPAddressConverter))); + s_typeConvertersRegistered = true; + }
159-162: Use Opts.SkipIndexScanOnReadsDefault to avoid duplicated defaultsThis default already exists in Opts. Referencing it here keeps a single source of truth.
- public bool SkipIndexScanOnReads { get; init; } = false; + public bool SkipIndexScanOnReads { get; init; } = Opts.SkipIndexScanOnReadsDefault;
376-379: Use Opts.HashCollisionReadLimitDefault for consistencyAvoid hardcoding defaults that already exist in Opts.
- public int HashCollisionReadLimit { get; init; } = 100; + public int HashCollisionReadLimit { get; init; } = Opts.HashCollisionReadLimitDefault;
563-568: Align interface threshold defaults with Opts constantsKeeps defaults centralized and consistent.
- public int ConnectionPendingSendBytesThreshold { get; init; } = 10 * 1_024 * 1_024; + public int ConnectionPendingSendBytesThreshold { get; init; } = Opts.ConnectionPendingSendBytesThresholdDefault; - public int ConnectionQueueSizeThreshold { get; init; } = 50_000; + public int ConnectionQueueSizeThreshold { get; init; } = Opts.ConnectionQueueSizeThresholdDefault;
602-605: Prefer Opts.ProjectionsQueryExpiryDefaultSame rationale: single source of truth.
- public int ProjectionsQueryExpiry { get; init; } = 5; + public int ProjectionsQueryExpiry { get; init; } = Opts.ProjectionsQueryExpiryDefault;
457-466: Avoid type/property name shadowing in WriteStatsToDb logicUsing the enum type name StatsStorage and the property StatsStorage side-by-side is confusing. Fully-qualify the enum or rename the property to reduce ambiguity. Also note that binding both WriteStatsToDb and StatsStorage could conflict; the last-applied binding wins.
Option A (minimal): fully-qualify the enum in this block.
- get => (StatsStorage.Stream & StatsStorage) != 0; + get => (EventStore.Core.Services.Monitoring.StatsStorage.Stream & StatsStorage) != 0; init => StatsStorage = - value - ? StatsStorage.StreamAndFile - : StatsStorage.File; // TODO SS: not sure if we should do this here + value + ? EventStore.Core.Services.Monitoring.StatsStorage.StreamAndFile + : EventStore.Core.Services.Monitoring.StatsStorage.File; // TODO SS: not sure if we should do this hereOption B (clearer): rename the property StatsStorage to StatsStorageMode in this record to avoid shadowing (larger change).
Also applies to: 475-476
610-617: Prefer init-only setters for options to preserve immutabilityMost options use init; these two use set. Unless you intend to mutate after construction, make them init for consistency.
- public int ProjectionCompilationTimeout { get; set; } = 500; + public int ProjectionCompilationTimeout { get; init; } = 500; - public int ProjectionExecutionTimeout { get; set; } = DefaultProjectionExecutionTimeout; + public int ProjectionExecutionTimeout { get; init; } = DefaultProjectionExecutionTimeout;
620-684: Improve unknown-key suggestions by comparing normalized keysLevenshtein distance on raw keys (mix of env, cli, and section formats) can degrade suggestions. Compute distance using normalized keys, but report stripped/original for UX.
- static (string UnknownKey, string SuggestedKey) CreateUnknownOptionResult(IEnumerable<string> knownKeys, - string unknownKey, int distanceThreshold = 5) + static (string UnknownKey, string SuggestedKey) CreateUnknownOptionResult(IEnumerable<string> knownKeys, + string unknownKey, int distanceThreshold = 5) { - var suggestion = knownKeys - .Select(key => (AllowedKey: key, Distance: Levenshtein.GetDistance(unknownKey, key))) + var normalizedUnknown = EventStoreConfigurationKeys.Normalize(unknownKey); + var suggestion = knownKeys + .Select(key => (AllowedKey: key, Distance: Levenshtein.GetDistance(normalizedUnknown, key))) .MinBy(x => x.Distance); return ( UnknownKey: EventStoreConfigurationKeys.StripConfigurationPrefix(unknownKey), SuggestedKey: suggestion.Distance > distanceThreshold ? "" : EventStoreConfigurationKeys.StripConfigurationPrefix(suggestion.AllowedKey) ); }
512-517: Remove the unusedGrpcOptions.FromConfigurationA global search for any calls to
GrpcOptions.FromConfiguration(…)returned no references outside its declaration inClusterVNodeOptions.cs:512–517, confirming it isn’t used elsewhere in the codebase. Binding of the gRPC settings already occurs viaGrpc = configuration.BindOptions<GrpcOptions>()in
ClusterVNodeOptions.FromConfiguration(IConfigurationRoot)(seeClusterVNodeOptions.cs:85) and in the Extension methods for reloading.Suggested refactor (optional):
- Delete the
internal static GrpcOptions FromConfiguration(IConfiguration configurationRoot)method to remove dead code.- OR, if you plan to expose a standalone factory, rename the parameter to
configurationSection, switch the implementation to:and document that callers must pass the specific “Grpc” section.internal static GrpcOptions FromConfiguration(IConfiguration configurationSection) { var opts = new GrpcOptions(); configurationSection.Bind(opts); return opts; }src/EventStore.Core/Configuration/ClusterVNodeOptionsExtensions.cs (2)
21-24: Spelling: “Plugable” vs “Pluggable” leaks into API surfaceThe term is commonly spelled “Pluggable.” If the interface/type names are under our control, consider renaming to avoid long‑term API debt.
Would you like me to draft a follow-up PR with type/member renames and Obsolete shims?
159-164: Missing/empty XML summary for AdvertiseNodeAsPlease clarify intent and precedence vs. AdvertiseExternalHostAs. Example:
- /// <summary> - /// </summary> + /// <summary> + /// Sets the advertised HTTP (node) host and port clients should use to reach the admin/UI/HTTP APIs. + /// Takes precedence over inferred values from bindings. Does not affect external TCP advertise fields. + /// </summary>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (18)
docker-compose.yml(3 hunks)samples/server/docker-compose.yaml(1 hunks)shared.env(1 hunks)src/EventStore.Core.Tests/Helpers/MiniClusterNode.cs(0 hunks)src/EventStore.Core.Tests/Helpers/MiniNode.cs(2 hunks)src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_cluster_node_and_custom_settings.cs(4 hunks)src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_default_settings.cs(0 hunks)src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_secure_tcp.cs(3 hunks)src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_single_node_and_custom_settings.cs(2 hunks)src/EventStore.Core.XUnit.Tests/Configuration/ClusterVNodeOptionsTests.cs(3 hunks)src/EventStore.Core.XUnit.Tests/Telemetry/TelemetryServiceTests.cs(9 hunks)src/EventStore.Core/Authentication/PassthroughAuthentication/PassthroughAuthenticationProvider.cs(1 hunks)src/EventStore.Core/ClusterVNode.cs(1 hunks)src/EventStore.Core/Configuration/ClusterVNodeOptions.Framework.cs(1 hunks)src/EventStore.Core/Configuration/ClusterVNodeOptions.cs(1 hunks)src/EventStore.Core/Configuration/ClusterVNodeOptionsExtensions.cs(1 hunks)src/EventStore.Core/Telemetry/TelemetryService.cs(7 hunks)src/EventStore.SystemRuntime/Diagnostics/DriveStats.cs(1 hunks)
💤 Files with no reviewable changes (2)
- src/EventStore.Core.Tests/Helpers/MiniClusterNode.cs
- src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_default_settings.cs
🚧 Files skipped from review as they are similar to previous changes (9)
- samples/server/docker-compose.yaml
- docker-compose.yml
- src/EventStore.Core/Authentication/PassthroughAuthentication/PassthroughAuthenticationProvider.cs
- src/EventStore.Core/Telemetry/TelemetryService.cs
- src/EventStore.Core.Tests/Helpers/MiniNode.cs
- src/EventStore.Core/ClusterVNode.cs
- src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_cluster_node_and_custom_settings.cs
- src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_single_node_and_custom_settings.cs
- shared.env
🧰 Additional context used
🧬 Code graph analysis (4)
src/EventStore.Core.XUnit.Tests/Configuration/ClusterVNodeOptionsTests.cs (3)
src/EventStore.Common.Tests/Configuration/ConfigurationRootExtensionsTest.cs (3)
Fact(10-24)Fact(26-39)Fact(41-54)src/EventStore.Core/Configuration/ClusterVNodeOptions.Framework.cs (1)
ClusterVNodeOptions(23-51)src/EventStore.Core/Configuration/TypeConverters.cs (1)
EndPoint(19-31)
src/EventStore.Core.XUnit.Tests/Telemetry/TelemetryServiceTests.cs (1)
src/EventStore.SystemRuntime/RuntimeInformation.cs (1)
RuntimeInformation(15-50)
src/EventStore.Core/Configuration/ClusterVNodeOptions.cs (4)
src/EventStore.Core/Configuration/ClusterVNodeOptionsExtensions.cs (16)
ClusterVNodeOptions(16-19)ClusterVNodeOptions(21-23)ClusterVNodeOptions(25-34)ClusterVNodeOptions(41-44)ClusterVNodeOptions(52-55)ClusterVNodeOptions(62-67)ClusterVNodeOptions(76-82)ClusterVNodeOptions(90-97)ClusterVNodeOptions(105-107)ClusterVNodeOptions(115-120)ClusterVNodeOptions(128-130)ClusterVNodeOptions(138-140)ClusterVNodeOptions(148-157)ClusterVNodeOptions(164-171)X509Certificate2(178-216)X509Certificate2Collection(227-273)src/EventStore.Core/Configuration/TypeConverters.cs (4)
EndPoint(19-31)GossipSeedConverter(34-57)GossipEndPointConverter(10-32)IPAddressConverter(59-67)src/EventStore.Core/Util/Opts.cs (1)
Opts(7-33)src/EventStore.Core/Configuration/Sources/EventStoreConfigurationKeys.cs (3)
EventStoreConfigurationKeys(13-94)EventStoreConfigurationKeys(26-40)Normalize(43-72)
src/EventStore.Core/Configuration/ClusterVNodeOptionsExtensions.cs (7)
src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_cluster_node_and_custom_settings.cs (9)
ClusterVNodeOptions(19-26)ClusterVNodeOptions(87-88)ClusterVNodeOptions(109-110)ClusterVNodeOptions(129-137)ClusterVNodeOptions(157-164)ClusterVNodeOptions(185-192)ClusterVNodeOptions(213-222)ClusterVNodeOptions(246-250)ClusterVNodeOptions(265-268)src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_secure_tcp.cs (2)
ClusterVNodeOptions(21-33)ClusterVNodeOptions(67-73)src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_single_node_and_custom_settings.cs (3)
ClusterVNodeOptions(20-20)ClusterVNodeOptions(39-40)ClusterVNodeOptions(57-63)src/EventStore.Core/Configuration/ClusterVNodeOptions.cs (2)
ClusterVNodeOptions(32-32)ClusterVNodeOptions(60-94)src/EventStore.Common/Utils/EndpointExtensions.cs (2)
GetHost(23-31)GetPort(43-50)src/EventStore.Core/Certificates/CertificateUtils.cs (1)
CertificateUtils(13-315)src/EventStore.Common/Utils/StringExtensions.cs (1)
IsNotEmptyString(14-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build / ubuntu-24.04
- GitHub Check: Build / ubuntu-24.04-arm
- GitHub Check: Build / bookworm-slim
- GitHub Check: Docker Compose Smoke Test
- GitHub Check: Build / noble
🔇 Additional comments (14)
src/EventStore.SystemRuntime/Diagnostics/DriveStats.cs (2)
31-35: Good resilience: logged failure with safe fallback.Catching exceptions, logging with context, and returning a sentinel value avoids cascading failures in diagnostics paths. LGTM.
28-28: Confirm and document free-space semantics for DriveData.AvailableBytesDriveStats.GetDriveInfo currently uses
DriveInfo.AvailableFreeSpace(user-quota aware) to populateDriveData.AvailableBytes. That choice propagates into:
- Metrics (SystemMetrics) →
DriveUsedBytes&DriveTotalBytes(usage = UsedBytes/TotalBytes)- Monitoring (SystemStatsHelper) →
sys-drive-…-availableBytes,usedBytes,usage,totalBytes- Telemetry (MachineInfo) → only
TotalBytes(no free-space metric)If your consumers expect raw, volume-level free space (ignoring per-account quotas), switch to
DriveInfo.TotalFreeSpace. Otherwise, to avoid confusion, please:
- Update the XML docs in DriveStats.GetDriveInfo and DriveData to explicitly state that
AvailableBytesis user-quota aware.- Consider adding a comment at the instantiation site to call out the choice.
Locations to update:
- src/EventStore.SystemRuntime/Diagnostics/DriveStats.cs (line 28 – constructor call)
- src/EventStore.SystemRuntime/Diagnostics/DriveStats.cs (lines 43–44 –
<param name="AvailableBytes">)Suggested diff for clarity:
--- a/src/EventStore.SystemRuntime/Diagnostics/DriveStats.cs +++ b/src/EventStore.SystemRuntime/Diagnostics/DriveStats.cs @@ public static DriveData GetDriveInfo(string path) - return new DriveData(diskName, info.TotalSize, info.AvailableFreeSpace); + // Using AvailableFreeSpace (user-quota aware). + // To measure raw, volume-level free space, replace with info.TotalFreeSpace. + return new DriveData( + diskName, + info.TotalSize, + info.AvailableFreeSpace + ); @@ <param name="AvailableBytes">The available free space on the disk in bytes.</param> -/// <param name="AvailableBytes">The available free space on the disk in bytes.</param> +/// <param name="AvailableBytes"> +/// The amount of free space available to this process’s account (user-quota aware). +/// Use DriveInfo.TotalFreeSpace for raw, volume-level free space. +/// </param>src/EventStore.Core.XUnit.Tests/Telemetry/TelemetryServiceTests.cs (2)
52-55: Good: IConfiguration injection for cloudIdentifierPassing IConfiguration into TelemetryService and supplying EventStore:Telemetry:CloudIdentifier via AddInMemoryCollection is clean and testable. This aligns the test with the new constructor.
213-221: Nice: test covers normalization to lowercasePublishing “Baz” and asserting “baz” in sink verifies the new normalization logic.
src/EventStore.Core.XUnit.Tests/Configuration/ClusterVNodeOptionsTests.cs (2)
173-190: Good coverage: array-based GossipSeed bindingThis test validates binding EndPoint[] from EventStore:GossipSeed:{index} keys. Nice to see both IP and DNS cases.
210-220: Good: environment variable binding for NodeIpValidates EVENTSTORE_NODE_IP → Interface.NodeIp. Straightforward and helpful regression coverage.
src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests/when_building/with_secure_tcp.cs (3)
24-24: API migration looks correctSwitch from WithInternalSecureTcpOn/WithExternalSecureTcpOn to WithReplicationEndpointOn/WithExternalTcpOn is applied correctly here.
70-72: Consistent endpoint updates with Secure()Chaining WithReplicationEndpointOn + WithExternalTcpOn before Secure(...) is consistent with the new API layout.
104-105: Potential IP conflation via WithExternalTcpOn (see extension implementation)WithExternalTcpOn currently sets Interface.NodeIp under the hood (see ClusterVNodeOptionsExtensions). If a test sets HTTP endpoint via WithNodeEndpointOn to a different IP, WithExternalTcpOn may overwrite it. Ensure call order reflects intent, or address this in the extension (see my comment on the extension file).
src/EventStore.Core/Configuration/ClusterVNodeOptions.cs (4)
350-351: QuorumSize computation looks correctFor single-node clusters quorum is 1; otherwise it’s majority (N/2 + 1). This is standard and matches expected semantics.
498-505: gRPC defaults look reasonableKeepAliveInterval/Timeout defaults at 10s are sane and match typical gRPC server keepalive configurations.
218-230: Auth options: backwards-compatibility flag documented and default-safeDisableFirstLevelHttpAuthorization is default false with clear description. Looks good.
70-88: LoadedOptions and GetLoadedOptions definitions confirmed
Both the LoadedOptions property and the GetLoadedOptions method are implemented in the companion partial:
- src/EventStore.Core/Configuration/ClusterVNodeOptions.Framework.cs:
• Line 58:public IReadOnlyDictionary<string, LoadedOption> LoadedOptions { get; init; } = new Dictionary<string, LoadedOption>();
• Line 82:public static IReadOnlyDictionary<string, LoadedOption> GetLoadedOptions(IConfigurationRoot configurationRoot) { … }No compilation issues—no changes needed.
src/EventStore.Core/Configuration/ClusterVNodeOptionsExtensions.cs (1)
115-121: Endpoint helpers look consistent; LGTM
- WithReplicationEndpointOn correctly sets ReplicationIp/Port.
- WithNodeEndpointOn correctly sets NodeIp/Port.
- AdvertiseInternalHostAs sets both ReplicationHostAdvertiseAs and ReplicationTcpPortAdvertiseAs.
No changes requested.
Also applies to: 128-131, 148-157
| public ClusterVNodeOptions() => FileStreamExtensions.ConfigureFlush(Database.UnsafeDisableFlushToDisk); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
ConfigureFlush runs before configuration is bound — final setting may be ignored
Calling ConfigureFlush in the constructor uses the default Database.UnsafeDisableFlushToDisk value (false) and doesn’t reflect the configuration-bound value. Ensure the final, bound value wins by invoking ConfigureFlush after binding in FromConfiguration. Keeping the constructor call is harmless if you also add the post-bind call (see next comment). If you prefer a single source of truth, remove it here.
Apply one of the following:
Option A (preferred): keep constructor as-is and add a post-bind call (see lines 93-94 comment).
Option B (remove here, rely only on post-bind):
-public ClusterVNodeOptions() => FileStreamExtensions.ConfigureFlush(Database.UnsafeDisableFlushToDisk);
+public ClusterVNodeOptions() { }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public ClusterVNodeOptions() => FileStreamExtensions.ConfigureFlush(Database.UnsafeDisableFlushToDisk); | |
| ++ b/src/EventStore.Core/Configuration/ClusterVNodeOptions.cs | |
| @@ Lines 32-33 | |
| public ClusterVNodeOptions() { } |
🤖 Prompt for AI Agents
In src/EventStore.Core/Configuration/ClusterVNodeOptions.cs around lines 32-33,
ConfigureFlush is being called in the constructor using the default
Database.UnsafeDisableFlushToDisk (so configuration-bound values may be
ignored); fix by ensuring ConfigureFlush is invoked after configuration binding
in FromConfiguration (around lines 93-94) so the bound value wins — preferred:
keep the constructor call and add a post-bind call to
FileStreamExtensions.ConfigureFlush(Database.UnsafeDisableFlushToDisk) in
FromConfiguration; alternatively, if you prefer a single source, remove the
constructor call and only call ConfigureFlush after binding.
| return options; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure flush-to-disk honors loaded configuration
After binding all option groups, set the FileStream flush behavior using the bound value. This guarantees the effective configuration is applied even if the constructor previously configured a default.
- return options;
+ // Ensure FileStream flush behavior matches the loaded configuration
+ FileStreamExtensions.ConfigureFlush(options.Database.UnsafeDisableFlushToDisk);
+ return options;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return options; | |
| } | |
| // Ensure FileStream flush behavior matches the loaded configuration | |
| FileStreamExtensions.ConfigureFlush(options.Database.UnsafeDisableFlushToDisk); | |
| return options; | |
| } |
🤖 Prompt for AI Agents
In src/EventStore.Core/Configuration/ClusterVNodeOptions.cs around lines 93-94,
after binding all option groups and before returning options, update the
FileStream flush-to-disk setting to use the bound configuration value (e.g.,
assign the bound flush-to-disk flag or call the FileStream options setter with
the bound value) so the effective configuration overrides any constructor
default; ensure this assignment uses the freshly bound options property and
occurs prior to the return.
| // Handle array-style options; currently only GossipSeed uses this | ||
| if (sourceDisplayName is "<UNKNOWN>" && value is null) | ||
| { | ||
| var parentPath = option.Value.Key; | ||
| var childValues = new List<string>(); | ||
|
|
||
| foreach (var childKey in provider.GetChildKeys([], parentPath)) | ||
| { | ||
| var absoluteChildKey = parentPath + ":" + childKey; | ||
| if (!provider.TryGet(absoluteChildKey, out var childValue) || childValue is null) continue; | ||
| childValues.Add(childValue); | ||
| sourceDisplayName = GetSourceDisplayName(absoluteChildKey, provider); | ||
| } | ||
|
|
||
| value = string.Join(", ", childValues); | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Array-style options handling is gated behind a successful TryGet; likely misses providers where parent has no value
The new block only runs if provider.TryGet(optionKey, out value) succeeded and value is null. For common providers (e.g., InMemory, environment), TryGet("EventStore:GossipSeed") returns false when only children exist (GossipSeed:0, GossipSeed:1), so this path is skipped and LoadedOptions will miss the setting.
Refactor to probe children when TryGet fails or value is null. Also set a hasValue flag and skip only if no value or children found.
Proposed refactor:
- foreach (var option in Metadata.SelectMany(x => x.Options)) {
- if (!provider.TryGet(option.Value.Key, out var value)) continue;
-
- var title = GetTitle(option);
- var sourceDisplayName = GetSourceDisplayName(option.Value.Key, provider);
- var isDefault = provider.GetType() == typeof(EventStoreDefaultValuesConfigurationProvider);
-
- // Handle array-style options; currently only GossipSeed uses this
- if (sourceDisplayName is "<UNKNOWN>" && value is null)
- {
- var parentPath = option.Value.Key;
- var childValues = new List<string>();
-
- foreach (var childKey in provider.GetChildKeys([], parentPath))
- {
- var absoluteChildKey = parentPath + ":" + childKey;
- if (!provider.TryGet(absoluteChildKey, out var childValue) || childValue is null) continue;
- childValues.Add(childValue);
- sourceDisplayName = GetSourceDisplayName(absoluteChildKey, provider);
- }
-
- value = string.Join(", ", childValues);
- }
-
- loadedOptions[option.Value.Key] = new(
+ foreach (var option in Metadata.SelectMany(x => x.Options)) {
+ var title = GetTitle(option);
+ var isDefault = provider.GetType() == typeof(EventStoreDefaultValuesConfigurationProvider);
+ string? value;
+ var hasValue = provider.TryGet(option.Value.Key, out value);
+ var sourceDisplayName = GetSourceDisplayName(option.Value.Key, provider);
+
+ if (!hasValue || value is null) {
+ var parentPath = option.Value.Key;
+ var childValues = new List<string>();
+ foreach (var childKey in provider.GetChildKeys([], parentPath)) {
+ var absoluteChildKey = parentPath + ":" + childKey;
+ if (!provider.TryGet(absoluteChildKey, out var childValue) || childValue is null) continue;
+ childValues.Add(childValue);
+ sourceDisplayName = GetSourceDisplayName(absoluteChildKey, provider);
+ hasValue = true;
+ }
+ if (childValues.Count > 0)
+ value = string.Join(", ", childValues);
+ }
+
+ if (!hasValue) continue;
+
+ loadedOptions[option.Value.Key] = new(
metadata: option.Value,
title: title,
value: value,
sourceDisplayName: sourceDisplayName,
isDefault: isDefault
);
}Consider adding a small unit test around GetLoadedOptions to assert that a provider with keys EventStore:GossipSeed:0/1 is surfaced with a comma-joined value in LoadedOptions.
🏁 Script executed:
#!/bin/bash
# Display the context around the foreach to verify current implementation
rg -nC3 'foreach\s*\(var option in Metadata\.SelectMany' src/EventStore.Core/Configuration/ClusterVNodeOptions.Framework.csLength of output: 434
🏁 Script executed:
#!/bin/bash
# Display the first 200 lines of the file to verify the context and ensure the snippet matches the review scope
sed -n '1,200p' src/EventStore.Core/Configuration/ClusterVNodeOptions.Framework.csLength of output: 7832
Refactor GetLoadedOptions to surface array-style options when the parent key has no direct value
The current implementation in src/EventStore.Core/Configuration/ClusterVNodeOptions.Framework.cs (around lines 96–112) only attempts to gather child values when
provider.TryGet(option.Value.Key, out var value)returns true—even if the parent key has no value. For providers like the environment or InMemory providers, TryGet("EventStore:GossipSeed") will return false when only children (GossipSeed:0, GossipSeed:1, etc.) exist, causing those array-style settings to be skipped entirely.
Refactor so that you:
- First call
TryGetto determine if the parent has a direct value - Always probe
GetChildKeyswhen either there was no direct value or it was null - Only skip adding the option if neither a direct value nor any children were found
Proposed diff:
@@ foreach (var option in Metadata.SelectMany(x => x.Options)) {
- if (!provider.TryGet(option.Value.Key, out var value)) continue;
- var title = GetTitle(option);
- var sourceDisplayName = GetSourceDisplayName(option.Value.Key, provider);
- var isDefault = provider.GetType() == typeof(EventStoreDefaultValuesConfigurationProvider);
- // Handle array-style options; currently only GossipSeed uses this
- if (sourceDisplayName is "<UNKNOWN>" && value is null)
- {
- var parentPath = option.Value.Key;
- var childValues = new List<string>();
- foreach (var childKey in provider.GetChildKeys([], parentPath))
- {
- var absoluteChildKey = parentPath + ":" + childKey;
- if (!provider.TryGet(absoluteChildKey, out var childValue) || childValue is null) continue;
- childValues.Add(childValue);
- sourceDisplayName = GetSourceDisplayName(absoluteChildKey, provider);
- }
- value = string.Join(", ", childValues);
- }
- loadedOptions[option.Value.Key] = new(
+ var title = GetTitle(option);
+ var isDefault = provider.GetType() == typeof(EventStoreDefaultValuesConfigurationProvider);
+ string? value;
+ var hasValue = provider.TryGet(option.Value.Key, out value);
+ var sourceDisplayName = GetSourceDisplayName(option.Value.Key, provider);
+ if (!hasValue || value is null) {
+ var parentPath = option.Value.Key;
+ var childValues = new List<string>();
+
+ foreach (var childKey in provider.GetChildKeys([], parentPath)) {
+ var absoluteChildKey = $"{parentPath}:{childKey}";
+ if (!provider.TryGet(absoluteChildKey, out var childValue) || childValue is null) continue;
+ childValues.Add(childValue);
+ sourceDisplayName = GetSourceDisplayName(absoluteChildKey, provider);
+ hasValue = true;
+ }
+
+ if (childValues.Count > 0)
+ value = string.Join(", ", childValues);
+ }
+
+ if (!hasValue) continue;
+
+ loadedOptions[option.Value.Key] = new(
metadata: option.Value,
title: title,
value: value,
sourceDisplayName: sourceDisplayName,
isDefault: isDefault
);Additionally, please add a small unit test for GetLoadedOptions that verifies a configuration provider containing keys like EventStore:GossipSeed:0 and EventStore:GossipSeed:1 yields a single entry for EventStore:GossipSeed with a comma-joined value.
🤖 Prompt for AI Agents
In src/EventStore.Core/Configuration/ClusterVNodeOptions.Framework.cs around
lines 96 to 112, the code only gathers child values when the parent key already
had a value, causing array-style settings (e.g., GossipSeed:0, GossipSeed:1) to
be skipped if the parent key has no direct value; change the logic to first
attempt provider.TryGet(option.Value.Key, out var value) to capture any direct
value, then always call provider.GetChildKeys(...) when value is null or TryGet
returned false to collect child entries, set sourceDisplayName from the first
child found, join child values into value if any exist, and only skip adding the
option when neither a direct value nor any child values were found; also add a
unit test for GetLoadedOptions that creates a config provider containing
EventStore:GossipSeed:0 and EventStore:GossipSeed:1 and asserts there is a
single loaded option for EventStore:GossipSeed whose value is the two entries
joined by a comma.
| public static ClusterVNodeOptions | ||
| AdvertiseExternalHostAs(this ClusterVNodeOptions options, EndPoint endPoint) => | ||
| options with { Interface = options.Interface with { NodeHostAdvertiseAs = endPoint.GetHost(), } }; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
AdvertiseExternalHostAs writes to NodeHostAdvertiseAs and collides with AdvertiseNodeAs
- AdvertiseExternalHostAs currently sets Interface.NodeHostAdvertiseAs (and drops the port entirely).
- AdvertiseNodeAs also sets Interface.NodeHostAdvertiseAs and NodePortAdvertiseAs.
- Using both in one configuration (as tests do) creates last-write-wins conflicts and loses the external advertise host/port.
Route external advertising to dedicated properties and include the port:
public static ClusterVNodeOptions
AdvertiseExternalHostAs(this ClusterVNodeOptions options, EndPoint endPoint) =>
- options with { Interface = options.Interface with { NodeHostAdvertiseAs = endPoint.GetHost(), } };
+ options with {
+ Interface = options.Interface with {
+ ExternalHostAdvertiseAs = endPoint.GetHost(),
+ ExternalTcpPortAdvertiseAs = endPoint.GetPort()
+ }
+ };Optionally, have AdvertiseNodeAs guard against accidental override when External* is already set, or document precedence rules in XML docs.
To locate potential conflicts in the repo:
Also applies to: 164-171
🏁 Script executed:
#!/bin/bash
# Spots places where both advertise helpers are used together
rg -nPU -C2 --type=cs '(?s)\.AdvertiseExternalHostAs\(.+?\).*?\.AdvertiseNodeAs\('
rg -nPU -C2 --type=cs '(?s)\.AdvertiseNodeAs\(.+?\).*?\.AdvertiseExternalHostAs\('
# Confirm whether dedicated External* advertise fields exist
rg -nP --type=cs '\bExternalHostAdvertiseAs\b|\bExternalTcpPortAdvertiseAs\b|\bNodeHostAdvertiseAs\b|\bNodePortAdvertiseAs\b'Length of output: 19291
Separate external-advertise settings from existing NodeHostAdvertiseAs
AdvertiseExternalHostAs and AdvertiseNodeAs both write to the same NodeHostAdvertiseAs/NodePortAdvertiseAs fields, causing last-write-wins collisions (and dropping the port in AdvertiseExternalHostAs). To correctly support distinct external/http advertise endpoints, we need to introduce dedicated External* properties, update the extension methods, and adjust the gossip-builder logic and tests accordingly.
Please address the following critical fixes:
• Add new properties in the Interface record (src/EventStore.Core/Configuration/ClusterVNodeOptions.cs):
– public string? ExternalHostAdvertiseAs { get; init; } = null;
– public int ExternalTcpPortAdvertiseAs { get; init; } = 0;
• Update AdvertiseExternalHostAs (src/EventStore.Core/Configuration/ClusterVNodeOptionsExtensions.cs, ~L138):
public static ClusterVNodeOptions
AdvertiseExternalHostAs(this ClusterVNodeOptions options, EndPoint endPoint) =>
- options with { Interface = options.Interface with { NodeHostAdvertiseAs = endPoint.GetHost(), } };
+ options with { Interface = options.Interface with {
+ ExternalHostAdvertiseAs = endPoint.GetHost(),
+ ExternalTcpPortAdvertiseAs = endPoint.GetPort()
+ }};• Optionally guard or document in AdvertiseNodeAs (src/EventStore.Core/Configuration/ClusterVNodeOptionsExtensions.cs, ~L169) to avoid overriding External* when already set, e.g.:
if (options.Interface.ExternalHostAdvertiseAs != null)
throw new InvalidOperationException("External advertise host already configured");—or add XML-doc rules on precedence.
• Adjust the gossip-builder in ClusterVNode (src/EventStore.Core/ClusterVNode.cs around L872 and L916) to prefer ExternalHostAdvertiseAs/ExternalTcpPortAdvertiseAs over NodeHostAdvertiseAs/NodePortAdvertiseAs when constructing the external HTTP endpoint.
• Update or add unit tests in src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests to verify:
– ExternalHostAdvertiseAs + ExternalTcpPortAdvertiseAs correctly populate GossipAdvertiseInfo.HttpEndPoint.
– AdvertiseNodeAs continues to set the internal cluster tcp advertise values.
These changes are required to avoid breaking existing behavior while correctly separating external HTTP advertise endpoints.
🤖 Prompt for AI Agents
In src/EventStore.Core/Configuration/ClusterVNodeOptionsExtensions.cs around
lines 138-141, AdvertiseExternalHostAs currently writes into
Interface.NodeHostAdvertiseAs causing collisions with AdvertiseNodeAs and
dropping the port; add new External* properties to the Interface record in
src/EventStore.Core/Configuration/ClusterVNodeOptions.cs (public string?
ExternalHostAdvertiseAs { get; init; } = null; and public int
ExternalTcpPortAdvertiseAs { get; init; } = 0), update AdvertiseExternalHostAs
to set these new ExternalHostAdvertiseAs/ExternalTcpPortAdvertiseAs fields
instead of the existing Node* fields, optionally add a guard in AdvertiseNodeAs
(around src/EventStore.Core/Configuration/ClusterVNodeOptionsExtensions.cs
~L169) to throw or document if External* are already set to avoid silent
overrides, modify the gossip-builder logic in
src/EventStore.Core/ClusterVNode.cs (around L872 and L916) to prefer
ExternalHostAdvertiseAs/ExternalTcpPortAdvertiseAs when composing the external
HTTP endpoint and fall back to NodeHostAdvertiseAs/NodePortAdvertiseAs
otherwise, and update/add unit tests in
src/EventStore.Core.XUnit.Tests/Configuration/ClusterNodeOptionsTests to assert
ExternalHostAdvertiseAs + ExternalTcpPortAdvertiseAs populate
GossipAdvertiseInfo.HttpEndPoint and that AdvertiseNodeAs still sets internal
cluster TCP advertise values.
| foreach (var candidate in DriveInfo.GetDrives()) | ||
| { | ||
| if (target.StartsWith(candidate.Name, StringComparison.InvariantCultureIgnoreCase) && | ||
| candidate.Name.StartsWith(diskName, StringComparison.InvariantCultureIgnoreCase)) | ||
| { | ||
| diskName = candidate.Name; | ||
| } | ||
| } | ||
|
|
||
| return new DriveData(diskName, info.TotalSize, info.AvailableFreeSpace); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure DiskName is non-empty (UNC/network paths, container mountpoints).
DriveInfo.GetDrives may not include UNC roots (e.g., \server\share) or certain container overlays, leaving diskName as "". Fall back to the detected root (target) to avoid empty DiskName in telemetry.
foreach (var candidate in DriveInfo.GetDrives())
{
if (target.StartsWith(candidate.Name, StringComparison.InvariantCultureIgnoreCase) &&
candidate.Name.StartsWith(diskName, StringComparison.InvariantCultureIgnoreCase))
{
diskName = candidate.Name;
}
}
+ if (string.IsNullOrEmpty(diskName))
+ diskName = target;
+
return new DriveData(diskName, info.TotalSize, info.AvailableFreeSpace);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| foreach (var candidate in DriveInfo.GetDrives()) | |
| { | |
| if (target.StartsWith(candidate.Name, StringComparison.InvariantCultureIgnoreCase) && | |
| candidate.Name.StartsWith(diskName, StringComparison.InvariantCultureIgnoreCase)) | |
| { | |
| diskName = candidate.Name; | |
| } | |
| } | |
| return new DriveData(diskName, info.TotalSize, info.AvailableFreeSpace); | |
| foreach (var candidate in DriveInfo.GetDrives()) | |
| { | |
| if (target.StartsWith(candidate.Name, StringComparison.InvariantCultureIgnoreCase) && | |
| candidate.Name.StartsWith(diskName, StringComparison.InvariantCultureIgnoreCase)) | |
| { | |
| diskName = candidate.Name; | |
| } | |
| } | |
| if (string.IsNullOrEmpty(diskName)) | |
| diskName = target; | |
| return new DriveData(diskName, info.TotalSize, info.AvailableFreeSpace); |
🤖 Prompt for AI Agents
In src/EventStore.SystemRuntime/Diagnostics/DriveStats.cs around lines 19 to 28,
the loop that matches DriveInfo entries can leave diskName empty for UNC/network
paths or container mountpoints; update the code to ensure DiskName is never
empty by falling back to the detected root: if diskName is null or empty after
the loop, set it to the target root (use Path.GetPathRoot(target) or the
original target if no root is available) before constructing DriveData so
telemetry always has a non-empty DiskName.
1 participant
Removed: Deprecated options
Changed: Docker samples (environment variables)
Changed: Telemetry (allow extra config data, normalize keys to lowercase)