Skip to content
This repository has been archived by the owner on Oct 12, 2023. It is now read-only.

Update api to remove responsability of the library to coerce the urls #3

Closed

Conversation

CaiqueMitsuoka
Copy link

@CaiqueMitsuoka CaiqueMitsuoka commented Aug 12, 2019

Add new trace option for external calls

The goal is to get insights about external calls and which one might be under performing.

New external call trace api

This PRs adds a new option on external calls tracer to change the metric name. This can improve how you track performance of you external calls.

The current API still works exactly before, it will set the metric name for the module + function name of your call if no :metric_name is sent. But when a :metric_name is passed, it will override the module + function name.

A :metric_name can be defined in two ways, the first one is a string or a {Module, :function} that will be called with the function args to return the string, allowing name to be defined at runtime. Below is the examples of usage of the @trace with the current and new apis.
The metric name includes the External/ and /all since they are necessary for NewRelic api to process the metric correctly.

defmodule ModuleExample do
  @trace {:query_all, category: :external}
  def query_all do
  end
  # Metric name: External/ModuleExample.query_all/all
  
  @trace {:query_users, category: :external, metric_name: "domain.net/users"}
  def query_users do
  end
  # Metric name: External/domain.net/users/all  
  
  @trace {:query_any_entity, category: :external, metric_name: {__MODULE__, :entity_name}}
  def query_any_entity(entity) do
  end
  # Metric name: External/domain.net/ENTITY_NAME_GIVEN_ON_RUNTIME/all    
  
  def entity_name(%{name: name} = _entity), do: "domain.net/#{name}"
end

This is helpful for HTTP clients implementations like HTTPoison.

defmodule Instrumented.HTTPClient do
  use HTTPoison.Base
  
  @trace {:request, category: :external, metric_name: {__MODULE__, :metric_name}}
  def request(method, url, body \\ "", headers \\ [], options \\ []) do
    super(method, url, body, headers, options)
  end

  def metric_name(_method, url, _body, _headers, _options) do
    case :http_uri.parse(url) do
      {:ok, {_, _, host, _, path, _}} -> "#{host}#{path}"
      {:error, _} -> url
    end
  end
end

@CaiqueMitsuoka CaiqueMitsuoka self-assigned this Aug 12, 2019
{id, parent_id},
{start_time, start_time_mono, end_time_mono}
)
__MODULE__.call(mfa, {name, category: :external, metric_name: metric_name}, pid, ids, times)
Copy link

@britto britto Aug 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will be safer if we keep a single call signature, then pattern match this name tuple internally. That way we avoid unwanted/dangerous recursions. Apart from that, this whole thing looks pretty good 👌.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another idea to consider: replacing this keyword list with a map, e.g. %{name: name, category: :external, metric_name: metric_name}. It's a definitely a bit more verbose, also a breaking change in the API, which would require careful deprecation, but IMO it paves the way for better extensibility.

@bhicks bhicks closed this Oct 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants