-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce resource_name_override option #14
Changes from all commits
4771cd9
2c8ef36
93b8cc3
3b3fb77
1a35cb5
68652f5
d46c387
708832f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -756,6 +756,71 @@ defmodule CarReqTest do | |
%{datadog_service_name: :guu_car_caz}} | ||
end | ||
|
||
test "allows client to set a resource_name_override rule" do | ||
defmodule ResourceNameOverrideClient do | ||
use CarReq, | ||
resource_name_override: &ResourceNameOverrideClient.resource_name_override/1 | ||
|
||
def resource_name_ovveride(resource) do | ||
String.replace(resource, ~r|\d+|, "{guid}") | ||
end | ||
end | ||
|
||
ResourceNameOverrideClient.request(method: :get, url: "/200", adapter: &Adapter.success/1) | ||
|
||
assert_receive {:event, [:http_car_req, :request, :start], _, | ||
%{resource_name_override: override}} | ||
|
||
assert is_function(override, 1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
end | ||
|
||
test "allows resource name override option to be set per-request on the client" do | ||
defmodule ResourceNameOverrideRuntimeClient do | ||
use CarReq, | ||
resource_name_override: &ResourceNameOverrideRuntimeClient.global_override/1 | ||
|
||
def request_override(resource_name) do | ||
String.replace(resource_name, ~r|\d+|, "{guid}") | ||
end | ||
|
||
def global_override(resource_name) do | ||
String.replace(resource_name, "/", "<slash>") | ||
end | ||
end | ||
|
||
url = "/employees/1234/details" | ||
|
||
ResourceNameOverrideRuntimeClient.request( | ||
method: :get, | ||
url: url, | ||
adapter: &Adapter.success/1, | ||
resource_name_override: &ResourceNameOverrideRuntimeClient.request_override/1 | ||
) | ||
Comment on lines
+793
to
+798
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, I think a test showing both the fn call and a static string would make sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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_fun}} | ||
|
||
assert override_fun.(url) == ResourceNameOverrideRuntimeClient.request_override(url) | ||
end | ||
|
||
test "allows a hard-coded string to replace resource_name on a per-request basis rather than a function" do | ||
defmodule StringResourceNameReplacementClient do | ||
use CarReq | ||
end | ||
|
||
StringResourceNameReplacementClient.request( | ||
method: :get, | ||
url: "employees/1234/details", | ||
adapter: &Adapter.success/1, | ||
resource_name_override: "get employees_details_endpoint" | ||
) | ||
|
||
assert_receive {:event, [:http_car_req, :request, :start], _, | ||
%{resource_name_override: override}} | ||
|
||
assert override == "get employees_details_endpoint" | ||
end | ||
|
||
test "determines service name for external namespaced clients" do | ||
defmodule Test.Foo.Bar.External.Service.ServiceNameClient do | ||
use CarReq | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case you didn't know, sometime ago we added support for
{x}
input_path_params
e.g.:So if
{x}
is the main use case for this perhaps and you're not using it yet, perhaps worth looking intopath_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.