HTTP Request Server Span semantic convention seems broken?

[sncariad]

Component(s)

exporter/azuremonitor

Describe the issue you're reporting

Semantic Conventions for HTTP Spans #Status states the following:

For HTTP status codes in the 4xx range span status MUST be left unset in case of SpanKind.SERVER and MUST be set to Error in case of SpanKind.CLIENT.

However Azure Monitor Exporter does not consider SpanKind when setting Success=false/true.

opentelemetry-collector-contrib/exporter/azuremonitorexporter/trace_to_envelope.go

Lines 698 to 702 in 103eb7b

	func getDefaultFormattedSpanStatus(spanStatus ptrace.Status) (statusCodeAsString string, success bool) {
	code := spanStatus.Code()
	return strconv.FormatInt(int64(code), 10), code != ptrace.StatusCodeError
	}

So in Log Analytics workspace, trace table all activities of Kind=Server with 404 gets Success=false. I believe it is incorrect and the code should consider SpanKind when deciding success or failure.

Expected Result

As per Semantic Convention for HTTP Span, when trace is of Server Kind, the 400 status codes are considered successful.

Actual Result

[github-actions]

Pinging code owners:

• exporter/azuremonitor: @pcwiese

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[crobert-1]

Hello @sncariad, thanks for filing and including the references! I believe there's currently some level of ambiguity before this can be declared a bug or not. It seems like we're dealing with an issue of deciding the correct way to convert data from pdata to the format AppInsights uses.

As you're showing, the data.ResultCode variable is initially being set to the span's status code, which will be 0, 1, or 2. However, later on in the same call stack, this same data.ResultCode is being set to the HTTP status.

opentelemetry-collector-contrib/exporter/azuremonitorexporter/trace_to_envelope.go

Line 368 in 103eb7b

data.ResultCode, data.Success = getFormattedHTTPStatusValues(attrs.HTTPStatusCode)

Note the comment on the RemoteDependencyData.ResultCode member that's being set here:

	// Result code of a dependency call. Examples are SQL error code and HTTP
	// status code.
	ResultCode string `json:"resultCode"`

So I believe the first question that needs answered here is whether the RemoteDependencyData struct's ResultCode member should be the span status, or HTTP status code? @pcwiese Do you have a thought here?

I realize that maybe a check should be added just in case in the method you've referenced, but no matter what value it's set to (or not set to) it will be overwritten in the current call stack you're hitting.

[rajkumar-rangaraj]

@sncariad This behavior is intentional. The Azure Monitor Exporter sets the status field to false when the HTTP status code exceeds 400. This approach differs from Semantic Conventions but is consistent with Azure Monitor’s practices. You can refer to a .NET implementation for more details -

https://github.com/Azure/azure-sdk-for-net/blob/bed75c756d48bb742254ba83b4c4536ad2e0cd56/sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Customizations/Models/RequestData.cs#L60

[crobert-1]

Thanks for the help here @rajkumar-rangaraj! It looks like setting the ResultCode and Success members by default lines up with Azure's expectations, and works as a placeholder in case it isn't set again later in the call stack.

Please let us know if you have any other questions @sncariad, I'm going to close for now.

[bunkrur]

@sncariad This behavior is intentional. The Azure Monitor Exporter sets the status field to false when the HTTP status code exceeds 400. This approach differs from Semantic Conventions but is consistent with Azure Monitor’s practices. You can refer to a .NET implementation for more**** details -

https://github.com/Azure/azure-sdk-for-net/blob/bed75c756d48bb742254ba83b4c4536ad2e0cd56/sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Customizations/Models/RequestData.cs#L60

Can you confirm the same for the scenario where we export via otlp to a collector and then into azure monitor via the golang azure monitor exporter?

Previously it was possible to override the success/failure within Azure App Insights SDK to allow 404s to be seen as successes. I've written a processor within a dotnet otel extension to try and recreate this functionality but are you saying that app insights will swap any client error to failure in otel regardless? @rajkumar-rangaraj

[sncariad]

@sncariad This behavior is intentional. The Azure Monitor Exporter sets the status field to false when the HTTP status code exceeds 400. This approach differs from Semantic Conventions but is consistent with Azure Monitor’s practices. You can refer to a .NET implementation for more details -

https://github.com/Azure/azure-sdk-for-net/blob/bed75c756d48bb742254ba83b4c4536ad2e0cd56/sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Customizations/Models/RequestData.cs#L60

Thanks for response and sharing the reference to .net version of exporter which does same as go version. But to my understanding; in either case they don't follow Semantic Conventions where they ignore to check if it is Request trace and not Dependency before considering Status to be Success or Failure for http status codes above 400.

Once again thank you for your support.

[crobert-1]

From @sncariad:

But to my understanding; in either case they don't follow Semantic Conventions where they ignore to check if it is Request trace and not Dependency before considering Status to be Success or Failure for http status codes above 400.

The Semantic Conventions referenced apply only to the OpenTelemetry data format. In the code here, data is being converted to the format used by AppInsights. What we're saying here is that the Semantic Conventions are not relevant once converted to this other format, since it's no longer within the OpenTelemetry realm.

If data was being exported using OTLP, then your point would be correct, the status code would need to be set to the values you're expecting. However, since it's being exported in this ApplicationInsights format, it follows different conventions.

Please let me know if I've misunderstood your comment here.

[bunkrur]

From @sncariad:

But to my understanding; in either case they don't follow Semantic Conventions where they ignore to check if it is Request trace and not Dependency before considering Status to be Success or Failure for http status codes above 400.

The Semantic Conventions referenced apply only to the OpenTelemetry data format. In the code here, data is being converted to the format used by AppInsights. What we're saying here is that the Semantic Conventions are not relevant once converted to this other format, since it's no longer within the OpenTelemetry realm.

If data was being exported using OTLP, then your point would be correct, the status code would need to be set to the values you're expecting. However, since it's being exported in this ApplicationInsights format, it follows different conventions.

Please let me know if I've misunderstood your comment here.

Can you confirm the same for the scenario where we export via otlp to a collector and then into azure monitor via the golang azure monitor exporter?

Like to really dumb it down, Applicaiton -> Otlp -> Otel Collector -> Azure Monitor Exporter would follow the OTLP format and not the in-code exporter format?

@crobert-1

[crobert-1]

Can you confirm the same for the scenario where we export via otlp to a collector and then into azure monitor via the golang azure monitor exporter?

Like to really dumb it down, Applicaiton -> Otlp -> Otel Collector -> Azure Monitor Exporter would follow the OTLP format and not the in-code exporter format?

Exporting in OTLP to the Otel Collector should follow the OTEL semantic conventions. The collector exporting data using the Azure Monitor exporter does not need to follow conventions. Does that make sense? Did I answer the question here?