yarpc / yarpc-go

A message passing platform for Go
MIT License
401 stars 101 forks source link

fix logged error in observability middleware when fields of transport.Request is in the `tagsBlocklist` #2229

Closed AllenLuUber closed 10 months ago

AllenLuUber commented 10 months ago

Let's say you have added routing-delegate in the tagsBlocklist, state of cache on first request this would be the map structure: [req.procedure+req.routingdelegate+...] - [metrics[tags - routing-delegate:"-", procedure:"req-procedure", ...]] For another request with different delegate key: [req.procedure+req.routingdelegate1+...] - [metrics[tags - routing-delegate:"-", procedure:"req-procedure", ...]] It ended up creating same metrics instances which results in failure from constructor from net/metrics.

In the PR, I was able to reproduce the issue using the added test before fixing the code:

--- FAIL: TestTagsBlocklistWithReservedField (0.00s)
... more errors
{{error 2023-08-30 19:25:54.629064475 +0000 UTC m=+0.002754291  Failed to create response payload size histogram. undefined } [{error %!s(zapcore.FieldType=25) %!s(int64=0)  a metric with name "response_payload_size_bytes" and the same constant tag names and values is already registered}]}]" should have 0 item(s), but has 12

The bug is we did not check from this tagsBlocklist when we are constructing the key for metrics reuse. It was always added. This diff makes it optional based on the values of tagsBlocklist. Also changed the type to maps for faster lookup.

codecov[bot] commented 10 months ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.08% :tada:

Comparison is base (efa54f1) 84.10% compared to head (e05334c) 84.19%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #2229 +/- ## ========================================== + Coverage 84.10% 84.19% +0.08% ========================================== Files 216 216 Lines 12886 12937 +51 ========================================== + Hits 10838 10892 +54 + Misses 1703 1701 -2 + Partials 345 344 -1 ``` | [Files Changed](https://app.codecov.io/gh/yarpc/yarpc-go/pull/2229?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yarpc) | Coverage Δ | | |---|---|---| | [internal/observability/graph.go](https://app.codecov.io/gh/yarpc/yarpc-go/pull/2229?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yarpc#diff-aW50ZXJuYWwvb2JzZXJ2YWJpbGl0eS9ncmFwaC5nbw==) | `94.60% <100.00%> (+1.48%)` | :arrow_up: | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/yarpc/yarpc-go/pull/2229/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yarpc)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.