MetricPoint public API changes

[cijothomas]

The numerical values are now retrived using LongValue, DoubleValue etc. These fields have meaning based on the type (eg: For histogram DoubleValue means the SUM, for Long Counter , DoubleValue means nothing and is an error if trrying to access)

Proposal is to make these internal, and instead provide methods like GetHistogramSum etc., so that exporters doesn't have to know any internal details. This will also allow us from changing the MetricPoint structure internally, w/o breaking exporters.

[joaopgrassi]

This would make things much clearer. I just now wrote an exporter and had to dig in the code to see what they meant.

[cijothomas]

This would make things much clearer. I just now wrote an exporter and had to dig in the code to see what they meant.

yes surely.! A bigger motivation is to hide the implementation from users, so we can optimize MetricPoint in the future, without breaking.

[joaopgrassi]

Just to maybe capture some feedback around this. I'm building an exporter and some things I stumbled across that might be considered:

• It would be nice if it was possible to know how many points a metric holds. Similar to this: Batch<T> public api additions to unblock users #2542, so I could allocate a list with the correct amount.

For example:

var lines = new List<string>(metric.PointsCount); // new API
foreach (var metricPoint in metric.GetMetricPoints())
{
	lines.add(SerializePoint(metricPoint))
}

• A nicer API around collecting the attributes. Right now we have to loop through metricPoint.Keys and metricPoint.Values. I thought I saw something around improving it, but couldn't find anymore. The issue is that exporters will probably have to duplicate code similar to this.

for (int i = 0; i < metricPoint.Keys.Length; i++)
{
	tagsBuilder.Append(metricPoint.Keys[i]);
	tagsBuilder.Append(':');
	tagsBuilder.Append(metricPoint.Values[i]);
	tagsBuilder.Append(' ');
}

• Validation when adding attributes. Now we accept <string, object?>, which allows me to pass something like an instance of a class. Is this already in our backlog to improve? I ask because you can do things like this (which was a surprise)

public class MyClass
{
	public override string ToString() => "OOPS";
}

// either this or by using `TagList`
var attributes = new KeyValuePair<string, object?>[]
{
	new ("key1", new MyClass()),
};

var attributes2 = new TagList
{
	{ "my_label", new MyClass() }
};

var counter = meter.CreateCounter<int>("counter");
counter.Add(10, attributes);

[cijothomas]

For attributes/tags - this pr #2642 is merged.

Regarding the tag value being object - Could you clarify what is the ask?

[cijothomas]

It would be nice if it was possible to know how many points a metric holds. --> we should be able to support this.

[joaopgrassi]

Regarding the tag value being object - Could you clarify what is the ask?

It's just that the spec says only primitive values (or lists of primitive values) are allowed, but here I can add whatever value I want. And in this case, I added an instance of a class and it took the ToString() out of it. We probably should allow only primitive types, for example like Java does https://github.com/open-telemetry/opentelemetry-java/blob/main/api/all/src/main/java/io/opentelemetry/api/common/AttributeKey.java

[cijothomas]

Closing the overall issue as its covered by several prs, and is part of 1.2.0-rc1 release.

Have few additional feedback from @joaopgrassi . Will operate specific issues for tracking them, to add post 1.2.0