Fix NullReferenceException caught by SDK when metric has a tag with a null value #3325

alanwest · 2022-06-01T19:02:21Z

When recording a metric with a null-valued tag the following code throws a NullReferenceException causing the measurement to be ignored. This code is invoked when locating the MetricPoint in the AggregationStore.

opentelemetry-dotnet/src/OpenTelemetry/Metrics/Tags.cs

Lines 87 to 90 in 56d6677

    
           for (int i = 0; i < this.Keys.Length; i++) 
        
           { 
        
               hash = (hash * 31) + this.Keys[i].GetHashCode() + this.Values[i].GetHashCode(); 
        
           }

The exception is caught here

opentelemetry-dotnet/src/OpenTelemetry/Metrics/AggregatorStore.cs

Lines 343 to 346 in 56d6677

    
           catch (Exception) 
        
           { 
        
               OpenTelemetrySdkEventSource.Log.MeasurementDropped(this.name, "SDK internal error occurred.", "Contact SDK owners."); 
        
           }

Fixed it so that measurements with null-valued tags are not ignored. The null-valued tag is passed through. There's some follow on work needed to ensure exporters can handle null values. See: #3325 (comment)

codecov · 2022-06-01T19:14:03Z

Codecov Report

Merging #3325 (a155e08) into main (5006370) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #3325   +/-   ##
=======================================
  Coverage   85.42%   85.43%           
=======================================
  Files         270      270           
  Lines        9560     9561    +1     
=======================================
+ Hits         8167     8168    +1     
  Misses       1393     1393

Impacted Files	Coverage Δ
src/OpenTelemetry/Metrics/Tags.cs	`73.07% <100.00%> (+1.07%)`	⬆️

cijothomas · 2022-06-02T16:29:20Z

good find.. need to see how other languages deal with this. (or is it only an issue in .net)...

reyang · 2022-06-02T18:41:37Z

I think our options are:

Ignore the measurement (basically what we do today). Though I think In this case we should improve on the log message.

Record the measurement without the null-valued tag. Maybe still log indicating that a tag was dropped?

Check/handle null value in GetHashCode and Equals. Exporters should expect to see attributes with null value, and they should be handled properly.

alanwest · 2022-06-02T20:30:51Z

good find.. need to see how other languages deal with this. (or is it only an issue in .net)...

I think it is in part a .NET-only issue given that our attributes can be of any object type. Though, it's possible other languages have run into this question for string values depending on how the language represents strings.

Check/handle null value in GetHashCode and Equals. Exporters should expect to see attributes with null value, and they should be handled properly.

👍 I'm good with making this an exporter concern.

Regarding null values, the spec says.

Attribute values of null are not valid and attempting to set a null value is undefined behavior.

So, it'd be up to us to decide how to handle them in the OTLP and Prometheus exporters.

OTLP

The proto has this comment

// The value is one of the listed fields. It is valid for all values to be unspecified
// in which case this AnyValue is considered to be "empty".

So our options would be:

New up an AnyValue whose value is uninitialized. We currently do this for null values encountered in array-valued attributes.
- I'm uncertain how this should translate when we introduce OTLP/JSON support. Maybe this is a spec question that already has an answer...
Drop the attribute. And log?

Prometheus

I'm less familiar with things here. The Prometheus format states:

label_value can be any sequence of UTF-8 characters, but the backslash (), double-quote ("), and line feed (\n) characters have to be escaped as \, ", and \n, respectively.

So should the value of a null-valued label just be ""? I think I recall reading somewhere that empty labels in Prometheus are equivalent to no label at all, but I can't recall where I saw this. If this is true, then it suggests we could just drop the label.

reyang · 2022-06-02T20:47:11Z

So should the value of a null-valued label just be ""? I think I recall reading somewhere that empty labels in Prometheus are equivalent to no label at all, but I can't recall where I saw this. If this is true, then it suggests we could just drop the label.

You've probably seen it from PR review 😃

opentelemetry-dotnet/src/OpenTelemetry.Exporter.Prometheus/Implementation/PrometheusSerializer.cs

Line 230 in fa21e1e

    
           // In Prometheus, a label with an empty label value is considered equivalent to a label that does not exist.

alanwest · 2022-06-02T22:05:10Z

You've probably seen it from PR review 😃

Ha, yes! It was this comment I saw. I saw it as I was expanding the tests on #3312.

alanwest · 2022-06-02T22:07:28Z

So I think this PR is good to go. I'll follow up and make sure the exporters handle null values ok. I suspect they might be ok because this was handled with my work with the tag transformer stuff.

cijothomas · 2022-06-02T22:11:39Z

test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs

@@ -42,6 +42,39 @@ public MetricApiTest(ITestOutputHelper output)
            this.output = output;
        }

+        [Fact]
+        public void MeasurementWithNullValuedTag()


cijothomas · 2022-06-02T22:12:24Z

So I think this PR is good to go. I'll follow up and make sure the exporters handle null values ok. I suspect they might be ok because this was handled with my work with the tag transformer stuff.

Do you want this to be part of 1.3.0 which I expect to release tomorrow.? If not, lets hold off merging for a day.

alanwest · 2022-06-02T22:15:24Z

Do you want this to be part of 1.3.0 which I expect to release tomorrow.? If not, lets hold off merging for a day.

It's probably not a common issue, so good to go after 1.3.0.

cijothomas · 2022-06-03T19:30:36Z

@alanwest could you fix conflict and we are good to merge.

tgrieger-sf · 2022-07-15T16:42:41Z

src/OpenTelemetry/Metrics/Tags.cs

+            unchecked
+            {
+                var hash = 17;
+                for (int i = 0; i < this.Keys.Length; i++)
+                {
+                    hash = (hash * 31) + this.Keys[i].GetHashCode() + this.Values[i]?.GetHashCode() ?? 0;
+                }
+
+                this.hashCode = hash;
+            }


Is there any concern that the underlying values of Keys or Values will get changed since the array indexes can still be written to but the hash won't be updated?

@tgrieger-sf No because the the keys and values never do change. This struct is internal and instances of it are only used within our AggregatorStore for looking up metric points

opentelemetry-dotnet/src/OpenTelemetry/Metrics/AggregatorStore.cs

Line 33 in a19d106

private readonly ConcurrentDictionary<Tags, int> tagsToMetricPointIndexDictionary =

.

If that's the case, is there any benefit in using ReadOnlySpan<string/object> for Keys and Values to make it explicit?

ReadOnlySpan is a ref struct and cannot be used a valid type for a dictionary as it has to live on stack.

Add test recording a metric with a null-valued dimension

fafaeed

alanwest requested a review from a team June 1, 2022 19:02

alanwest added 3 commits June 2, 2022 14:35

Fix null-valued tags

da6b615

Assert.Null instead of Assert.Equal

4a421ec

Compute hash code in constructor

bfe2c3e

alanwest changed the title ~~What should we do when a measurement has a null-valued tag?~~ Fix NullReferenceException caught by SDK when metric has a tag with a null value Jun 2, 2022

alanwest added 2 commits June 2, 2022 14:58

Update changelog

3c38408

Merge branch 'main' into alanwest/null-valued-dimensions

82f20af

cijothomas reviewed Jun 2, 2022

View reviewed changes

cijothomas approved these changes Jun 2, 2022

View reviewed changes

reyang approved these changes Jun 2, 2022

View reviewed changes

utpilla approved these changes Jun 3, 2022

View reviewed changes

alanwest and others added 2 commits June 3, 2022 12:33

Merge branch 'main' into alanwest/null-valued-dimensions

3d57791

Merge branch 'main' into alanwest/null-valued-dimensions

a155e08

cijothomas merged commit a2aa305 into open-telemetry:main Jun 3, 2022

alanwest deleted the alanwest/null-valued-dimensions branch June 6, 2022 17:19

tgrieger-sf reviewed Jul 15, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix NullReferenceException caught by SDK when metric has a tag with a null value #3325

Fix NullReferenceException caught by SDK when metric has a tag with a null value #3325

alanwest commented Jun 1, 2022 •

edited

Loading

codecov bot commented Jun 1, 2022 •

edited

Loading

cijothomas commented Jun 2, 2022

reyang commented Jun 2, 2022

alanwest commented Jun 2, 2022

reyang commented Jun 2, 2022

alanwest commented Jun 2, 2022

alanwest commented Jun 2, 2022

cijothomas Jun 2, 2022

cijothomas commented Jun 2, 2022

alanwest commented Jun 2, 2022

cijothomas commented Jun 3, 2022

tgrieger-sf Jul 15, 2022

alanwest Jul 15, 2022

tgrieger-sf Jul 16, 2022

utpilla Jul 18, 2022

	for (int i = 0; i < this.Keys.Length; i++)
	{
	hash = (hash * 31) + this.Keys[i].GetHashCode() + this.Values[i].GetHashCode();
	}

	catch (Exception)
	{
	OpenTelemetrySdkEventSource.Log.MeasurementDropped(this.name, "SDK internal error occurred.", "Contact SDK owners.");
	}

Fix NullReferenceException caught by SDK when metric has a tag with a null value #3325

Fix NullReferenceException caught by SDK when metric has a tag with a null value #3325

Conversation

alanwest commented Jun 1, 2022 • edited Loading

codecov bot commented Jun 1, 2022 • edited Loading

Codecov Report

cijothomas commented Jun 2, 2022

reyang commented Jun 2, 2022

alanwest commented Jun 2, 2022

OTLP

Prometheus

reyang commented Jun 2, 2022

alanwest commented Jun 2, 2022

alanwest commented Jun 2, 2022

cijothomas Jun 2, 2022

Choose a reason for hiding this comment

cijothomas commented Jun 2, 2022

alanwest commented Jun 2, 2022

cijothomas commented Jun 3, 2022

tgrieger-sf Jul 15, 2022

Choose a reason for hiding this comment

alanwest Jul 15, 2022

Choose a reason for hiding this comment

tgrieger-sf Jul 16, 2022

Choose a reason for hiding this comment

utpilla Jul 18, 2022

Choose a reason for hiding this comment

alanwest commented Jun 1, 2022 •

edited

Loading

codecov bot commented Jun 1, 2022 •

edited

Loading