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

feat: add new event metadata to tesla telemetry #454

Conversation

polvalente
Copy link
Contributor

This PR aims to provide a way for relaying through the tesla.request.stop event the unresolved path when using both Telemetry and PathParams middlewares.

The issue with the current implementation is that in success cases, it's impossible to retrieve the unresolved path from the event metadata

@teamon
Copy link
Member

teamon commented Mar 24, 2021

I see. Assuming you use Telemetry middleware before PathParams I think the best way would be to use something similar to KeepRequest:

defmodule KeepRequestURL do
  def call(env, next, _opts) do
    env
    |> Tesla.put_opt(:req_url, env.url)
    |> Tesla.run(next)
  end
end

# then in client
plug Tesla.Middleware.Telemetry
plug Tesla.Middleware.KeepRequestURL
plug Tesla.Middleware.PathParams

Then in you telemetry handlers you should be able to access the original URL via env.opts[:req_url].

I'd be happy to accept a PR adding :req_url to the built-in KeepRequest middleware too.

@polvalente
Copy link
Contributor Author

I see. Assuming you use Telemetry middleware before PathParams I think the best way would be to use something similar to KeepRequest:

defmodule KeepRequestURL do
  def call(env, next, _opts) do
    env
    |> Tesla.put_opt(:req_url, env.url)
    |> Tesla.run(next)
  end
end

# then in client
plug Tesla.Middleware.Telemetry
plug Tesla.Middleware.KeepRequestURL
plug Tesla.Middleware.PathParams

Then in you telemetry handlers you should be able to access the original URL via env.opts[:req_url].

I'd be happy to accept a PR adding :req_url to the built-in KeepRequest middleware too.

Yeah, the Telemetry middleware is before PathParams, but without this fix the URL ends up being resolved for happy-path scenarios, and thus this PR.

I'll change this PR to adding :req_url to the KeepRequest middleware then, a I'll also take the time to add an example to the Telemetry docs.

Thanks for the review and the suggestion!

@polvalente
Copy link
Contributor Author

@teamon the PR has now been updated as per your suggestion. I took the liberty to add an "integration" test for KeepRequest + PathParams + Telemetry, as well as an example for other Tesla.Middleware.Telemetry users.

If there's a better place to add these, please let me know!

@polvalente polvalente force-pushed the fix/also-keep-previous-env-for-handling-path-params branch from 8b17990 to f3e96dd Compare March 25, 2021 03:57
@teamon
Copy link
Member

teamon commented Mar 25, 2021

Nice! 😍

@teamon teamon merged commit c76de37 into elixir-tesla:master Mar 25, 2021
@polvalente polvalente deleted the fix/also-keep-previous-env-for-handling-path-params branch March 25, 2021 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants