You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "ptupitsyn (via GitHub)" <gi...@apache.org> on 2025/07/08 16:31:28 UTC
[PR] IGNITE-25163 .NET: Fix MetricsTests flakiness [ignite-3]
ptupitsyn opened a new pull request, #6221:
URL: https://github.com/apache/ignite-3/pull/6221
Add RecordObservableInstruments and WaitForCondition to AssertTaggedMetric.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25163 .NET: Fix MetricsTests flakiness [ignite-3]
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
ptupitsyn merged PR #6221:
URL: https://github.com/apache/ignite-3/pull/6221
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25163 .NET: Fix MetricsTests flakiness [ignite-3]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #6221:
URL: https://github.com/apache/ignite-3/pull/6221#discussion_r2192966380
##########
modules/platforms/dotnet/Apache.Ignite.Tests/MetricsTests.cs:
##########
@@ -397,22 +416,13 @@ public void AssertMetric(string name, int value, int timeoutMs = 1000)
messageFactory: () => $"{name}: expected '{value}', but was '{GetMetric(name)}'");
}
- public void AssertTaggedMetric(string name, int value, string nodeAddr, Guid? clientId)
+ public void AssertTaggedMetric(string name, int value, string nodeAddr, Guid? clientId, int timeoutMs = 1000)
{
- if (clientId == null)
- {
- // Client id is not known, find by name and node address.
- var val = _metricsWithTags.Single(x =>
- x.Key.StartsWith($"{name}_{MetricTags.ClientId}=", StringComparison.Ordinal) &&
- x.Key.EndsWith($",{MetricTags.NodeAddress}={nodeAddr}", StringComparison.Ordinal));
-
- Assert.AreEqual(value, val.Value);
- }
- else
- {
- var taggedName = $"{name}_{MetricTags.ClientId}={clientId},{MetricTags.NodeAddress}={nodeAddr}";
- Assert.AreEqual(value, _metricsWithTags[taggedName]);
- }
+ TestUtils.WaitForCondition(
+ condition: () => GetTaggedMetric(name, nodeAddr, clientId) == value,
+ timeoutMs: timeoutMs,
+ messageFactory: () => $"{name} for {nodeAddr} ({clientId}): expected '{value}', " +
+ $"but was '{GetTaggedMetric(name, nodeAddr, clientId)}'");
Review Comment:
[nitpick] Avoid calling `GetTaggedMetric` twice (in the condition and the message). Capture its result in a local variable and reuse it to ensure consistency and reduce duplicate work.
```suggestion
int taggedMetric = GetTaggedMetric(name, nodeAddr, clientId);
TestUtils.WaitForCondition(
condition: () => taggedMetric == value,
timeoutMs: timeoutMs,
messageFactory: () => $"{name} for {nodeAddr} ({clientId}): expected '{value}', " +
$"but was '{taggedMetric}'");
```
##########
modules/platforms/dotnet/Apache.Ignite.Tests/MetricsTests.cs:
##########
@@ -389,6 +389,25 @@ public int GetMetric(string name)
return _metrics.TryGetValue(name, out var val) ? (int)val : 0;
}
+ public int GetTaggedMetric(string name, string nodeAddr, Guid? clientId)
+ {
+ _listener.RecordObservableInstruments();
+
+ if (clientId != null)
+ {
+ var taggedName = $"{name}_{MetricTags.ClientId}={clientId},{MetricTags.NodeAddress}={nodeAddr}";
+ return _metricsWithTags.TryGetValue(taggedName, out var val) ? (int)val : 0;
+ }
+
+ // Client id is not known, find by name and node address.
+ return _metricsWithTags
+ .Where(x =>
+ x.Key.StartsWith($"{name}_{MetricTags.ClientId}=", StringComparison.Ordinal) &&
+ x.Key.EndsWith($",{MetricTags.NodeAddress}={nodeAddr}", StringComparison.Ordinal))
+ .Select(x => (int)x.Value)
+ .SingleOrDefault();
Review Comment:
Using `SingleOrDefault` will return 0 when the expected tagged metric is missing, potentially masking issues. Consider using `Single()` to throw an exception if the metric isn't found.
```suggestion
.Single();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org