otelhttp: set unit on all instruments

[ash2k]

Currently the histogram metric doesn't have units specified. It's good to have them explicitly specified.

The recorded value is actually milliseconds, not microseconds:

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

Lines 230 to 233 in bead7e4

	// Use floating point division here for higher precision (instead of Millisecond method).
	elapsedTime := float64(time.Since(requestStartTime)) / float64(time.Millisecond)
	h.valueRecorders[ServerLatency].Record(ctx, elapsedTime, o)

[codecov]

Codecov Report

Merging #4500 (ea1784d) into main (badf346) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@          Coverage Diff          @@
##            main   #4500   +/-   ##
=====================================
  Coverage   80.9%   80.9%           
=====================================
  Files        150     150           
  Lines      10292   10304   +12     
=====================================
+ Hits        8327    8339   +12     
  Misses      1825    1825           
  Partials     140     140           

Files	Coverage Δ
instrumentation/net/http/otelhttp/common.go	100.0% <ø> (ø)
instrumentation/net/http/otelhttp/handler.go	87.7% <100.0%> (+0.7%)	⬆️

[pellared]

Can you set units for all of the metric instruments?
How about also adding description?
Reference: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.17.0/specification/metrics/semantic_conventions/http-metrics.md

[ash2k]

@pellared Done. I think the spec and what is actually implemented is not the same thing because the spec talks about compressed payload but the implementation measures uncompressed data read/written by the app. This is probably fine, but perhaps the spec needs to be augmented with metrics for uncompressed data. Also, some description language in the spec seem to be not written grammatically correct (from what I can tell, not a native speaker).

[ash2k]

Side note/question: it's really weird that maps are used when just normal fields could be. What is the reason? Map lookup involves computing the hash of the string key, etc and is definitely more work for the CPU vs just a pointer dereference.

[pellared]

Side note/question: it's really weird that maps are used when just normal fields could be. What is the reason? Map lookup involves computing the hash of the string key, etc and is definitely more work for the CPU vs just a pointer dereference.

Yeah, I saw this as well and I agree 👍 I created #4502

[pellared]

@pellared Done.

Thank you ❤️

I think the spec and what is actually implemented is not the same thing because the spec talks about compressed payload but the implementation measures uncompressed data read/written by the app. This is probably fine, but perhaps the spec needs to be augmented with metrics for uncompressed data. Also, some description language in the spec seem to be not written grammatically correct (from what I can tell, not a native speaker).

I think the current main version is a lot better: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md. Thank you for all the remarks 👍