Introduce resource_name_override option #14
Conversation
…e focused on what equals what
| ResourceNameOverrideRuntimeClient.request( | ||
| method: :get, | ||
| url: url, | ||
| adapter: &Adapter.success/1, | ||
| resource_name_override: &ResourceNameOverrideRuntimeClient.request_override/1 | ||
| ) |
There was a problem hiding this comment.
at the point of the request, does this need to be a function or could this also accept a string?
There was a problem hiding this comment.
I did not even consider that, but yes it could be and that probably makes sense in a lot of cases (i.e., we want the resource name to be static rather than dynamic). I'll make a change to slip this in too.
There was a problem hiding this comment.
Nice, I think a test showing both the fn call and a static string would make sense.
There was a problem hiding this comment.
Just added a test case and expanded the docs example to show the static string usage.
| assert_receive {:event, [:http_car_req, :request, :start], _, | ||
| %{resource_name_override: override}} | ||
|
|
||
| assert is_function(override, 1) |
There was a problem hiding this comment.
Sorry if this is obvious but I wonder why we're storing the override function in telemetry event metadata. It must mean that some other call will evaluate this function, replace the computed resource name with this override. Was it expensive or we didn't have enough data to figure out the resource name when emitting the event? In other words, the event would always have a :resource_name metadata key, use CarReq, resource_name_override: ... would update it before emitting. If that'd be possible I think we'd remove some coupling, i.e. that other part of the system wouldn't have to know about resource_name_override idea. I'm most likely missing something very obvious about the architecture but I thought I'd bring this up anyway!
| placeholder `{guid}`. For example | ||
|
|
||
| MyResourceOverrideClient.override_function("get partner.api.com/product/1234/details") | ||
| # => "get partner.api.com/product/{guid}/details" |
There was a problem hiding this comment.
In case you didn't know, sometime ago we added support for {x} in put_path_params e.g.:
Req.get!(
"https://httpbin.org/status/{code}",
path_params_style: :curly,
path_params: [code: 201]
).status
#=> 201So if {x} is the main use case for this perhaps and you're not using it yet, perhaps worth looking into path_params feature? A word of warning, there is a limitation with path params and query strings, wojtekmach/req#424, which could come up when integrating with some services, though.
If you dont' extra flexibility perhaps sticking to :x/{x} would work but otherwise, yeah, a function with overrides sounds good to me.
This PR introduces a new option to allow for a function that can be passed to telemetry_metadata called
resource_name_override.Why?
In our particular case, we generally have Datadog's default quantization rules apply to external URLs with well-formed GUIDs and such; however, we have a few clients where the default quantization rules to not apply and we end up with an explosion of resources in our APM backend (Datadog). This can cause things like noisy alerts because a single failure on a single endpoint can look like a 100% error rate if there has only been one total request made.
We can address this in one of two ways -- at our instrumentation level or by configuring our datadog agents with replacements rules. I tend to lean to wanting to do this specifically at the instrumentation level because we as developers have closer control of the rules and it does not require redeploying and configuring the Datadog agents. This PR allows us to control the resource_name replacement on a per-client and per-request basis, giving us flexibility to override resource_names at the application level when we define clients or introduce new requests.