Status Improvements

[CodeBlanch]

Fixes #1571.

Change: Fix StatusCode being sent as integer

I noticed this helping someone out on Gitter:

• We're sending StatusCode (otel.status_code) as the enum integer value.
• We're sending the Description (otel.status_description) on every http span (set to the reason phrase if it is available).

The Status spec defines the string names and doesn't mention the numeric values.

New version looks like this:

• We send the string representation of the StatusCode (otel.status_code).
• We no longer send the Description (otel.status_description) on every http span. My reasoning for that:
  1. If http reason phrase was part of the spec, it would be on http semantic conventions.
  2. .NET Core HttpResonse doesn't even have StatusDescription, because it isn't part of HTTP/2.

Note: We will continue to send a Description (otel.status_description) for http spans if an exception is thrown.

Change: JaegerExporter will now set error flag

Jaeger spec actually has a way to indicate that a span errored out. With the previous Status spec, it was really hard to know what was an error, so we didn't set the flag. But now we have a dedicated Error status so we can turn this on:

Change: ZipkinExporter will now set error flag

Zipkin spec actually has a way to indicate that a span errored out. With the previous Status spec, it was really hard to know what was an error, so we didn't set the flag. But now we have a dedicated Error status so we can turn this on:

TODOs

• CHANGELOG.md updated for non-trivial changes

[codecov]

Codecov Report

Merging #1579 (f169502) into master (b247b3d) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1579      +/-   ##
==========================================
+ Coverage   80.68%   80.74%   +0.05%     
==========================================
  Files         241      242       +1     
  Lines        6525     6544      +19     
==========================================
+ Hits         5265     5284      +19     
  Misses       1260     1260              

Impacted Files	Coverage Δ
...Telemetry.Api/Internal/ActivityHelperExtensions.cs	100.00% <100.00%> (ø)
src/OpenTelemetry.Api/Internal/SpanHelper.cs	100.00% <100.00%> (ø)
src/OpenTelemetry.Api/Internal/StatusHelper.cs	100.00% <100.00%> (ø)
src/OpenTelemetry.Api/Trace/ActivityExtensions.cs	94.11% <100.00%> (+0.78%)	⬆️
....Jaeger/Implementation/JaegerActivityExtensions.cs	96.12% <100.00%> (+0.06%)	⬆️
...metryProtocol/Implementation/ActivityExtensions.cs	86.84% <100.00%> (+0.05%)	⬆️
...plementation/ZipkinActivityConversionExtensions.cs	98.70% <100.00%> (+0.03%)	⬆️
...umentation.AspNet/Implementation/HttpInListener.cs	89.10% <100.00%> (-0.11%)	⬇️
...tation.AspNetCore/Implementation/HttpInListener.cs	86.56% <100.00%> (-0.30%)	⬇️
...tp/Implementation/HttpHandlerDiagnosticListener.cs	72.83% <100.00%> (-0.98%)	⬇️
... and 1 more

[reyang]

LGTM.

[CodeBlanch]

For anyone curious, I did benchmark switch statement vs dictionary lookup for the conversions to/from StatusCode/string. The lower you are in the switch, the slower the perf. But with the 3/4 cases, it's always faster than the dictionary.

Method	StatusCode	Mean	Error	StdDev	Median
DictionaryLookup	Error	17.956 ns	0.3061 ns	0.3525 ns	17.789 ns
SwitchLookup	Error	5.803 ns	0.0816 ns	0.0724 ns	5.840 ns
DictionaryLookup	Ok	15.594 ns	0.1286 ns	0.1074 ns	15.593 ns
SwitchLookup	Ok	3.236 ns	0.0424 ns	0.0376 ns	3.235 ns
DictionaryLookup	Unknown	13.100 ns	0.1269 ns	0.1125 ns	13.101 ns
SwitchLookup	Unknown	4.957 ns	0.1240 ns	0.1613 ns	4.890 ns
DictionaryLookup	Unset	18.120 ns	0.3561 ns	0.3331 ns	18.072 ns
SwitchLookup	Unset	7.551 ns	0.0665 ns	0.0556 ns	7.559 ns

[reyang]

@CodeBlanch I guess the string comparison call might be the biggest contributor to the switch perf.

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public static int? GetStatusCodeForStringName(string statusCodeName)
        {
            return statusCodeName switch
            {
                "Unset" => 0,
                "Error" => 2,
                "Ok" => 1,
                _ => null,
            };
        }

would translate to:

	IL_0000: ldarg.0
	IL_0001: brfalse.s IL_004a

	IL_0003: ldarg.0
	IL_0004: ldstr "Unset"
	IL_0009: call bool [System.Runtime]System.String::op_Equality(string, string)
	IL_000e: brtrue.s IL_002c

	IL_0010: ldarg.0
	IL_0011: ldstr "Error"
	IL_0016: call bool [System.Runtime]System.String::op_Equality(string, string)
	IL_001b: brtrue.s IL_0036

	IL_001d: ldarg.0
	IL_001e: ldstr "Ok"
	IL_0023: call bool [System.Runtime]System.String::op_Equality(string, string)
	IL_0028: brtrue.s IL_0040

[reyang]

So if the goal is to get the maximum performance, we can potentially look at the following steps:

1. avoid map/string hash at all.
2. avoid string comparison API, given the string is very short - and we could also avoid going through the locale (e.g. Ordinal)

[CodeBlanch]

Thanks @reyang I'll see what I can do to squeeze a bit more perf out of it.