Update otelfiber metrics implementation
#409
Conversation
Also adds the status code
|
@onematchfox Seems like tests are failing |
Ooops... My bad! Should all be working now. |
|
@onematchfox Can you bump the semconv version too? I ran into this issue about metrics being reported on / a couple days a go. So this PR should help with that. |
Bumped version as requested. I have however left the metric attributes as is for now. Do you also want to tackle aligning these with the latest version of the spec as well in this PR? For reference see |
onematchfox
requested review from
gaby
and removed request for
ReneWerner87 and
efectn
|
@onematchfox Given that we are using the latest opentelemetry, makes sense to also update the metrics attributes in this PR. |
onematchfox
force-pushed
the
metrics
branch
2 times, most recently
from
fb27ab5 to
52bff88
Compare
…12.0` Includes: - making `http.server_name` optional - this attribute is only ever mentioned as an example in the "[Observability Primer](https://opentelemetry.io/docs/concepts/observability-primer/)" (see https://opentelemetry.io/search/?q=http.server_name) and seems to have been replaced-by usage of `net.host.name` instead. - replacing `http.host` with `net.host.name` - improved accuracy of `http.scheme` through use of `ctx.Protocol()` instead of `ctx.Context().IsTLS()` - improved accuracy of `http.flavor` - addition of optional attribute `net.host.port` See https://opentelemetry.io/docs/reference/specification/metrics/semantic_conventions/http-metrics/ for more
…2.0` Includes: - making `http.server_name` optional - this attribute is only ever mentioned as an example in the "[Observability Primer](https://opentelemetry.io/docs/concepts/observability-primer/)" (see https://opentelemetry.io/search/?q=http.server_name) and seems to have been replaced-by usage of `[net.host.name](https://opentelemetry.io/docs/reference/specification/trace/semantic_conventions/span-general/#nethostname)` instead. - addition of recommended attributes `http.flavor` and `http.response_content_length` - addition of conditionally required attribute `net.host.port` - removed optional/unmentioned attribute `net.host.ip`; this was being incorrectly set to the client IP instead of the the host. Client IP is already available via `http.client_ip`. See https://opentelemetry.io/docs/reference/specification/trace/semantic_conventions/http/ for more
All done now. Suspect there's a few thing we may need to discuss :). Most pertinently, I wasn't quite sure how you wanted to handle the "removal" of |
|
@gaby can you check again, after your approvement i will merge it |
|
Sure, will do! |
Whilst trying out the metrics implementation added in #385 I noticed that all metrics were being reported on the route
/. This is because the route was being accessed prior toc.Next()being called. As per the docs (and the existing tracing implementation) this can be only be set later in the method.So, first and foremost, this PR sets out to resolve that issue. In doing so I decided to use the opportunity to add the status code attribute as per the recommendations and, to prevent issues in future, expanded the tests to assert that the metrics are being produced as expected.
On a related note, I noticed that:
v1.4.0of thesemconvpackage (here and here). Given that it is referencing the latest versions of the main libraries I wonder if this shouldn't also be updated to the latest version (v1.12.0) as well?semconvmodule) don't actually conform with the semantic conventions spec for either metrics and or traces. E.g. the use ofhttp.server_nameovernet.host.nameFWIW, I did initially consider bundling changes to the attribute names to comply with the latest spec into this PR but figured those changes probably deserved a separate PR as I'm sure there is some discussion to be had.