Skip to content
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

Merged
merged 8 commits into from
Oct 21, 2024
42 changes: 42 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,48 @@ defmodule ExampleImpl do
use CarReq, datadog_service_name: :dont_put_me_in_a_box
end
```

### Resource Name Overrides

In most cases, your application's instrumentation modules (the ones that call `:telemetry.attach/4` on Telemetry events executed by CarReq) will take the metadata provided by CarReq's Telemetry events and form a resource name that may look something like:

```
get some_partner.api.com/product/4123/details
```

In some cases, you may want specific requests or clients to override the default resource name provided by your application's instrumentation module. To do so, you can pass along a `:resource_name_override` option at the client level or on a per-request
basis. The value of `:resource_name_override` must be a 1-arity function that takes a string (the default resource_name provided by your
instrumentation) and returns a new string (the overridden resource_name). Note that when passing this option at the client-level, you
must pass the option as an external function capture, i.e., `&Module.function/1`. An example would be:

```elixir
defmodule MyResourceOverrideClient do
use CarReq, resource_name_override: &MyResourceOverrideClient.override_function/1

@doc """
Replaces any consecutive instances of numbers in a string with the
placeholder `{guid}`. For example

MyResourceOverrideClient.override_function("get partner.api.com/product/1234/details")
# => "get partner.api.com/product/{guid}/details"

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} in put_path_params e.g.:

Req.get!(
  "https://httpbin.org/status/{code}",
  path_params_style: :curly,
  path_params: [code: 201]
).status
#=> 201

So 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.

"""
def override_function(resource_name) do
String.replace(resource_name, ~r|\d+|, "{guid}")
end
end
```

You can also provide an override function on a per-request basis as follows:

```elixir
MyResourceOverrideClient.request(
method: :get,
url: url,
resource_name_override: fn resource_name ->
String.replace(resource_name, ~r|\d+|, "{guid}")
end
```

## Options

# adapter
Expand Down
10 changes: 9 additions & 1 deletion lib/car_req.ex
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ defmodule CarReq do
],
fuse_melt_func: [
type: {:fun, 1}
],
resource_name_override: [
type: {:fun, 1}
]
]

Expand Down Expand Up @@ -183,7 +186,8 @@ defmodule CarReq do
Req.new()
|> Req.Request.register_options([
:datadog_service_name,
:implementing_module
:implementing_module,
:resource_name_override
])
|> LogStep.attach()
|> CarReq.attach_circuit_breaker(options)
Expand Down Expand Up @@ -268,12 +272,16 @@ defmodule CarReq do
|> Keyword.merge(request_options)
end

resource_name_override = Keyword.get(opts, :resource_name_override)

defp telemetry_metadata(request_options) do
%{
datadog_service_name:
Keyword.get(request_options, :datadog_service_name, @datadog_service_name),
url: Keyword.get(request_options, :url),
method: Keyword.get(request_options, :method),
resource_name_override:
Keyword.get(request_options, :resource_name_override, unquote(resource_name_override)),
query_params: Keyword.get(request_options, :params)
}
end
Expand Down
47 changes: 47 additions & 0 deletions test/car_req_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -756,6 +756,53 @@ 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)
Copy link

@wojtekmach wojtekmach Oct 21, 2024

Choose a reason for hiding this comment

The 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 :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!

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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 "determines service name for external namespaced clients" do
defmodule Test.Foo.Bar.External.Service.ServiceNameClient do
use CarReq
Expand Down
Loading