Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Akka.Cluster.Metrics mis-uses code-genned Protobuf objects for internal business logic #7065

Closed
Aaronontheweb opened this issue Jan 17, 2024 · 1 comment · Fixed by #7067
Closed

Comments

@Aaronontheweb
Copy link
Member

Aaronontheweb commented Jan 17, 2024

Per the blockers we ran into on #7063

public sealed partial class NodeMetrics
{
public Actor.Address Address { get; private set; }
/// <summary>
/// Creates new instance of <see cref="NodeMetrics"/>
/// </summary>
/// <param name="address">Address of the node the metrics are gathered at</param>
/// <param name="timestamp">the time of sampling, in milliseconds since midnight, January 1, 1970 UTC</param>
/// <param name="metrics">The set of sampled <see cref="Types.Metric"/></param>
public NodeMetrics(Actor.Address address, long timestamp, IEnumerable<Types.Metric> metrics)
{
Address = address;
timestamp_ = timestamp;
metrics_ = new RepeatedField<Types.Metric>();
metrics_.AddRange(metrics);
}
/// <summary>
/// Returns the most recent data.
/// </summary>
public NodeMetrics Merge(NodeMetrics that)
{
if (!Address.Equals(that.Address))
throw new ArgumentException(nameof(that), $"merge only allowed for same address, {Address} != {that.Address}");
if (Timestamp >= that.Timestamp)
return this; // that is order
return new NodeMetrics(Address, that.Timestamp, that.Metrics.Union(Metrics));
}
/// <summary>
/// Returns the most recent data with <see cref="Types.EWMA"/> averaging.
/// </summary>
public NodeMetrics Update(NodeMetrics that)
{
if (!Address.Equals(that.Address))
throw new ArgumentException(nameof(that), $"merge only allowed for same address, {Address} != {that.Address}");
// Apply sample ordering
var (latestNode, currentNode) = Timestamp >= that.Timestamp ? (this, that) : (that, this);
// Average metrics present in both latest and current.
var updated = latestNode.Metrics
.SelectMany(latest => currentNode.Metrics.Select(current => (Latest: latest, Current: current)))
.Where(pair => pair.Latest.SameAs(pair.Current))
.Select(pair => pair.Current + pair.Latest)
.ToList();
// Append metrics missing from either latest or current.
// Equality is based on the metric's name
var merged = updated.Union(latestNode.Metrics).Union(currentNode.Metrics);
return new NodeMetrics(Address, latestNode.Timestamp, merged);
}
/// <summary>
/// Gets metric by key
/// </summary>
public Option<Types.Metric> Metric(string name) => Metrics.FirstOrDefault(m => m.Name == name) ?? Option<Types.Metric>.None;
/// <summary>
/// Returns true if <code>that</code> address is the same as this
/// </summary>
public bool SameAs(NodeMetrics that) => Address.Equals(that.Address);
/*
* Two methods below, Equals and GetHashCode, should be used instead of generated in ClusterMetrics.Messages.g.cs
* file. Since we do not have an option to not generate those methods for this particular class,
* just stip them from generated code and paste here, with adding Address property check
*/
public bool Equals(NodeMetrics other)
{
if (ReferenceEquals(null, other)) return false;
if (ReferenceEquals(this, other)) return true;
return Equals(Address, other.Address);
}
public override int GetHashCode()
{
return (Address != null ? Address.GetHashCode() : 0);
}
}

Indeed, we are abusing the Protobuf objects inside our internal usages within Akka.Cluster.Metrics. That's a separate issue that I think will need to be solved in a different PR.

Originally posted by @Aaronontheweb in #7063 (comment)

@Aaronontheweb
Copy link
Member Author

I think the solution that is required here is decoupling the wire types from the public API and creating some structs that are actually returned by the NodeMetrics APIs et al.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant