Servlet instrumentation overwrites setStatus that was set manually earlier

[ryanrupp]

Describe the bug
See Slack thread here. I have some custom instrumentation that is trying to call span.setStatus(error) on a span generated by servlet instrumentation - this works correctly. However, the servlet instrumentation ends up overwriting this status with UNSET here due to HttpStatusConverter returning unset for >= 100 && < 400 HTTP status codes. In the Slack thread it's mentioned the instrumentation should only ever set ERROR. Note my HTTP request actually does return a 200 (non-error) but this is the nature of the RPC protocol I'm instrumenting that it returns 200 with an error status in the response body instead of HTTP status code.

Steps to reproduce
Add an instrumentation that modifies the span that the servlet is going to operate on, calling setStatus(error).

What did you expect to see?
The span should be reported with status=error

What did you see instead?
The span was reported with status=unset

What version are you using?
1.1.0

Environment
OpenJDK 1.8

[ryanrupp]

There's probably some other areas that are using this similar pattern of setting to UNSET explicitly instead of skipping the method span.setStatus method call, general pattern:

span.setStatus(mapFromException(something))

mapFromException(something) {
     return isError(something) ? ERROR : UNSET;
}

Other examples if you look for SpanStatus.UNSET usage e.g.:

https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/apache-dubbo/apache-dubbo-2.7/library/src/main/java/io/opentelemetry/instrumentation/apachedubbo/v2_7/DubboHelper.java#L27

opentelemetry-java-instrumentation/instrumentation/grpc-1.5/library/src/main/java/io/opentelemetry/instrumentation/grpc/v1_5/GrpcHelper.java

Line 36 in 0b0516b

return grpcCode.equals(Code.OK) ? StatusCode.UNSET : StatusCode.ERROR;

Discussion around:

1. Should span.setStatus ignore UNSET being passed?
2. Otherwise maybe something in BaseTracer that handles the ignoring aspect:

setStatus(span, status) {
    if (status == UNSET) {
         return;
    }

    span.setStatus(status)
}

[iNikem]

As I am going to submit a PR to spec soon to implement open-telemetry/opentelemetry-specification#1584 (comment), I can also submit a proposal that SDK should ignore Span.setStatus calls with UNSET. Let's see how the community will react to that.