Add request count metric with status code label

[nabokihms]

Signed-off-by: m.nabokikh [email protected]

Closes #711

This PR adds a counter for the total amount of requests with the http_status_code label. It will help us to be aware of the ratio of errors.

I decided to avoid adding status code to other metrics because it looks excessive. Instead, counting the number of requests seems more sensible.

P.S.:
I accidentally closed the previous PR #771

[nabokihms]

@Aneurysm9 fyi

[MrAlias]

This metric overlaps with the information that will be communicated by the http.server.active_requests: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.9.0/specification/metrics/semantic_conventions/http-metrics.md#http-server

I'm hesitant to accept this addition if we are ultimately going to comply with the OTel semantic conventions and there will be duplication. Should this be implemented as an UpDownCounter and track the active requests? Do the semantic conventions need to be updated before we add this?

[codecov]

Codecov Report

Merging #2138 (4501944) into main (e915b7c) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #2138   +/-   ##
=====================================
  Coverage   69.9%   69.9%           
=====================================
  Files        147     147           
  Lines       6934    6936    +2     
=====================================
+ Hits        4851    4853    +2     
  Misses      1959    1959           
  Partials     124     124           

Impacted Files	Coverage Δ
instrumentation/net/http/otelhttp/handler.go	82.8% <100.0%> (+0.1%)	⬆️

[Aneurysm9]

The spec recommends that the status code be added to the duration metrics, which can provide a count when aggregated as a histogram. Should we do that and rely on the user to configure a view to do dimension reduction if histograms with status codes is a cardinality concern?

[nabokihms]

From my point of view, it would be better to avoid using updown counter for errors, because you cannot apply rate functions to it.

Adding status code label for histograms sounds ok to me. It is a common practice. Should we go this way then?

[nabokihms]

@MrAlias @Aneurysm9, just a friendly reminder. Is it ok to add the status code label to the server latency metric?

[Aneurysm9]

@MrAlias @Aneurysm9, just a friendly reminder. Is it ok to add the status code label to the server latency metric?

I'm good with that approach.

[nabokihms]

I've just added the status code label, so everything should be good now.

[nabokihms]

Folks, any updates on this one? It would be best if we resolve all concerns about this PR before the next release 🙏

[nabokihms]

I rebased this branch on the main. Just a friendly reminder, still waiting for the feedback 😸

[sagikazarmark]

Any update on this PR?

[mmanciop]

Already implemented:

opentelemetry-go-contrib/instrumentation/net/http/otelhttp/handler.go

Line 246 in c52452d

attributes = append(attributes, semconv.HTTPStatusCode(statusCode))

[dmathieu]

Already implemented:

Your link is setting attributes for the spans, not metrics.

[nabokihms]

@dmathieu hello there! A test is not included for this PR. Do you want me to add one, or should I wait for the initial review?

[dmathieu]

I believe adding a test would make the review easier.

[nabokihms]

@dmathieu I checked this out, and it seems like the issue is fixed by the following code

opentelemetry-go-contrib/instrumentation/net/http/otelhttp/handler.go

Lines 219 to 221 in c52452d

	if rww.statusCode > 0 {
	attributes = append(attributes, semconv.HTTPStatusCode(rww.statusCode))
	}

All thanks to @MrAlias. Now everything works as expected. Thanks to everyone. It was a two years journey that came to an end (I'm going to close the PR).