feat: add new metadata to Datadog traces #1763
Conversation
Because API responses are always 200, even if there's an error message in the body of the response, it's hard to get metrics from Datadog about the health of services. What you see in Datadog traces: * The service name (default 'web' for Phoenix applications) is the name of the OMG application. * Traces are tagged with application version number * For API responses which error, the trace is flagged red as an error, the error code is marked on the trace and the error message is found in the tags.
| def add_trace_metadata(conn) do | ||
| case Jason.decode(conn.resp_body) do | ||
| {:ok, json_resp_body} -> add_metadata_from_response_body(conn, json_resp_body) | ||
| {:error, _} -> SpandexPhoenix.default_metadata(conn) | ||
| end | ||
| end |
There was a problem hiding this comment.
it seems to me that we could find a way that would avoid this serialization and deserialization.
if you look at OMG.WatcherRPC.Web.api_response/3 this gets called when success == true
if you look at OMG.WatcherRPC.Web.Controller.Fallback.call/2 this gets called when success == false
and you could attach metadata at this point in time with assigns https://elixircasts.io/exploring-phoenix-assigns
try it, let me know :)))
There was a problem hiding this comment.
I agree that it would be great to avoid decoding the response body back to JSON. I'll take a look at using Phoenix assigns instead. Thanks for the suggestion!
| type(:atom) | ||
| """ | ||
| defp add_metadata_from_response_body(conn, json_resp_body) do | ||
| service = String.to_atom(json_resp_body["service_name"]) |
There was a problem hiding this comment.
this seems somewhat dangerous that could peraps blow up the VM unintentionally. try to choose the atom, for example:
case (json_resp_body["service_name"] do
"watcher"->:watcher
"watcher_info"-> :watcher_info
end
the reason being, atoms are not garbage collected and turning strings to atoms is something we should avoid.
| error = if !json_resp_body["success"], do: Keyword.new([{:error, true}]) | ||
| error_type = if !!error and json_resp_body["data"], do: json_resp_body["data"]["code"] | ||
| error_msg = if !!error and json_resp_body["data"], do: json_resp_body["data"]["description"] |
There was a problem hiding this comment.
if clauses are to be used only with functions with questionmarks. in elixir, we prefer patter matching with case clauses
| _ = if error_type, do: tags ++ {String.to_atom("error.type"), error_type} | ||
| _ = if error_msg, do: tags ++ {String.to_atom("error.msg"), error_msg} |
There was a problem hiding this comment.
as said, String.to_atom is to be avoided whenever possible.
| if error, | ||
| do: | ||
| trace_data | ||
| |> Keyword.put(:error, error), | ||
| else: trace_data |
There was a problem hiding this comment.
this is a short if notation for a multiline statement... anyhow, use case clauses as they're prefered.
| Handles failure to decode the response body gracefully. | ||
| """ | ||
|
|
||
| def add_trace_metadata(conn) do |
There was a problem hiding this comment.
public functions on top, private at the bottom
| Phoenix.ConnTest.build_conn(:get, "/alerts.get") | ||
| |> Plug.Conn.resp(200, resp_body) |
There was a problem hiding this comment.
piping should be used for multiple statements piped.
yes:
:get
|> Phoenix.ConnTest.build_conn("/alerts.get")
|> Plug.Conn.resp(200, resp_body)
|
Thanks for the review @InoMurko. I'll close this PR and open a new one. There's too much to fix here in one pass. I'll re-use |
Overview
Because API responses are always 200, even if there's an error message in the body of the response, observability is impacted and it's hard to understand the health of services. This PR adds extra fields to the spans of tags passing through the Watcher and Watcher Info Phoenix based APIs, which flags responses with errors in the trace in Datadog.
While Datadog generates metrics from traces, unfortunately it isn't possible to group the traces by error_type in the metrics count (see https://docs.datadoghq.com/tracing/guide/metrics_namespace/#errors), so we will need to add more instrumentation, counting errors hit with custom metrics. This'll be a different PR.
Changes
What you see in Datadog traces after this change:
This is implemented by using
customize_metadataon the SpandexPhoenix tracer: https://hexdocs.pm/spandex_phoenix/SpandexPhoenix.html, parsing the JSON response body with Jason (hopefully a relatively cheap operation), and adding the fields to the default metadata.Testing
Make API calls to the endpoints, and check the local-development environment APM in Datadog. Look for services "watcher" and "watcher_info", and check the traces in the DataDog GUI.
P.S.
This is my first Elixir PR. Please let me know how my code can be more idiomatic!