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.

Sign up for GitHub

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

use router dispatch start events to update name of span #1

Closed
wants to merge 2 commits into from
Closed

use router dispatch start events to update name of span #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e750fd6
use router dispatch start events to update name of span
tsloughter May 9, 2020
d5bca1a
remove event_prefix from setup args
tsloughter May 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 24 additions & 12 deletions lib/opentelemetry_plug.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ defmodule OpentelemetryPlug do

Example:

OpentelemetryPlug.setup([:my, :plug])
OpentelemetryPlug.setup([])

You may also supply the following options in the second argument:

Expand All @@ -23,38 +23,44 @@ defmodule OpentelemetryPlug do
defaults to the concatenation of the event name with periods, e.g.
`"my.plug.start"`.
"""
def setup(event_prefix, config \\ []) do
def setup(config \\ []) do
# register the tracer. just re-registers if called for multiple repos
_ = OpenTelemetry.register_application_tracer(:opentelemetry_plug)

:telemetry.attach({__MODULE__, {event_prefix, :opentelemetry_phoenix_tracer_router_start}},
[:phoenix, :router_dispatch, :start],
&__MODULE__.handle_route/4,
config)

:telemetry.attach({__MODULE__, {event_prefix, :opentelemetry_plug_tracer_router_start}},
[:plug, :router_dispatch, :start],
&__MODULE__.handle_route/4,
config)

:telemetry.attach({__MODULE__, {event_prefix, :opentelemetry_plug_tracer_start}},
event_prefix ++ [:start],
[:plug_adapter, :call, :start],
&__MODULE__.handle_start/4,
config)

:telemetry.attach({__MODULE__, {event_prefix, :opentelemetry_plug_tracer_stop}},
event_prefix ++ [:stop],
[:plug_adapter, :call, :stop],
&__MODULE__.handle_stop/4,
config)

:telemetry.attach({__MODULE__, {event_prefix, :opentelemetry_plug_tracer_exception}},
event_prefix ++ [:exception],
[:plug_adapter, :call, :exception],
&__MODULE__.handle_exception/4,
config)
end
Comment on lines +30 to 54
Copy link
Contributor

Choose a reason for hiding this comment

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

Now you're relying on [:plug_adapter, :call], reckon you should remove your event_prefix argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, think I wasn't sure if it would be used for anything else, like adding support for Plug.Telemetry as well. But will remove it until that is added, if ever.


@doc false
def handle_start(_, _measurements, %{conn: conn}, _config) do
# TODO: add config for what paths are traced
Copy link
Contributor

Choose a reason for hiding this comment

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

Strikes me you could save yourself a lot of logic if you took a (Conn.t() -> boolean()) | {module(), atom()} filter option to call to find out whether to trace or not.


# setup OpenTelemetry context based on request headers
:ot_propagation.http_extract(conn.req_headers)

# TODO: add config option to allow `conn.request_path` as span name
span_name = case Map.get(conn.private, :plug_route) do
nil ->
"HTTP " <> conn.method
{route, _fun} ->
route
end
span_name = "HTTP " <> conn.method
Copy link
Contributor

Choose a reason for hiding this comment

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

… though I'm betting people will beg to change the span name, too…


{_, adapter} = conn.adapter
user_agent = header_or_empty(conn, "User-Agent")
Expand All @@ -79,6 +85,12 @@ defmodule OpentelemetryPlug do
OpenTelemetry.Tracer.start_span(span_name, %{attributes: attributes})
Copy link
Contributor

Choose a reason for hiding this comment

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

… and also the attributes eg. to handle RFC 7239 Forwarded headers as well as X-Forwarded-For, so by here I'm trying to come up with a clean way of letting them express the whole lot.

Assuming we trust them to propagate — handy if they need to accept other headers! — let's expose methods to determine attributes and to start spans with out tracer, and then call them back?

  # our code
  def handle_start(_, _, %{conn: conn}, %{custom_start: custom_start}) do
    custom_start.(conn)
  rescue
    _ -> :any_plans_for_how_to_stop_one_mistake_from_turning_off_tracing_forever?
  end

  @doc "Call `OpenTelemetry.Tracer.start_span/2` with our tracer, not yours."
  def start_span(span_name, start_opts), do:
    OpenTelemetry.Tracer.start_span(span_name, start_opts)
  # their code
  def my_custom_start(conn, start_span) do
    if my_should_trace?(conn) do
      my_propagate_from(conn.req_headers)
      OpentelemetryPlug.start_span(span_name, %{attributes: my_attributes(conn)})
    end
  end

  defp my_should_trace?(conn) do
    # checks the method or route, whatever
  end

  defp my_propagate_from(conn) do
    # calls :ot_propagation.http_extract/1 if it sees a traceparent
    # ensures equivalent if it sees another header
  end

  defp my_attributes(conn) do
    # calls OpentelemetryPlug.default_attributes/1
    # pipes it through Map.merge/2 with some more attributes
  end

Copy link
Contributor

Choose a reason for hiding this comment

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

Extracted to #3 so you don't need to ponder it as part of this PR.

end

@doc false
def handle_route(_, _measurements, %{route: route}, _config) do
# TODO: add config option to allow `conn.request_path` as span name
OpenTelemetry.Span.update_name(route)
end

@doc false
def handle_stop(_, _measurements, %{conn: conn}, _config) do
OpenTelemetry.Span.set_attribute("http.status", conn.status)
Expand Down
2 changes: 1 addition & 1 deletion mix.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ defmodule OpentelemetryPlug.MixProject do
# Run "mix help deps" to learn about dependencies.
defp deps do
[
{:plug, "~> 1.10"},
{:plug, github: "elixir-plug/plug", branch: "master"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{:plug, github: "elixir-plug/plug", branch: "master"},
{:plug, "~> 1.10.1"},

{:opentelemetry_api, "~> 0.3"},
{:opentelemetry, "~> 0.3"},
{:telemetry, "~> 0.4"}
Expand Down
2 changes: 1 addition & 1 deletion mix.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"mime": {:hex, :mime, "1.3.1", "30ce04ab3175b6ad0bdce0035cba77bba68b813d523d1aac73d9781b4d193cf8", [:mix], [], "hexpm", "6cbe761d6a0ca5a31a0931bf4c63204bceb64538e664a8ecf784a9a6f3b875f1"},
"opentelemetry": {:hex, :opentelemetry, "0.4.0", "293729f014009b03a1a2c47e6367db6f280b41412faa5639f06dcce9733d18a6", [:rebar3], [{:opentelemetry_api, "~> 0.3.1", [hex: :opentelemetry_api, repo: "hexpm", optional: false]}], "hexpm", "f0eb4281f26879147322b0a6f1c457c57f3f1c4121cbff1f6056e4c98b1647a7"},
"opentelemetry_api": {:hex, :opentelemetry_api, "0.3.1", "aa042f9ff0b774c3e9827e215fcf972d5cfccd9a79ed55194b58c329a945b486", [:mix, :rebar3], [], "hexpm", "91fc78c521b9fc7f72f47144bb9f0838d57584b1be5bbd3098d8aaf2f2d42686"},
"plug": {:hex, :plug, "1.10.0", "6508295cbeb4c654860845fb95260737e4a8838d34d115ad76cd487584e2fc4d", [:mix], [{:mime, "~> 1.0", [hex: :mime, repo: "hexpm", optional: false]}, {:plug_crypto, "~> 1.1.1 or ~> 1.2", [hex: :plug_crypto, repo: "hexpm", optional: false]}, {:telemetry, "~> 0.4", [hex: :telemetry, repo: "hexpm", optional: true]}], "hexpm", "422a9727e667be1bf5ab1de03be6fa0ad67b775b2d84ed908f3264415ef29d4a"},
"plug": {:git, "https://github.com/elixir-plug/plug.git", "9c880dee5a3d55c81f97a87076e540a9b5878cbd", [branch: "master"]},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"plug": {:git, "https://github.com/elixir-plug/plug.git", "9c880dee5a3d55c81f97a87076e540a9b5878cbd", [branch: "master"]},
"plug": {:hex, :plug, "1.10.1", "c56a6d9da7042d581159bcbaef873ba9d87f15dce85420b0d287bca19f40f9bd", [:mix], [{:mime, "~> 1.0", [hex: :mime, repo: "hexpm", optional: false]}, {:plug_crypto, "~> 1.1.1 or ~> 1.2", [hex: :plug_crypto, repo: "hexpm", optional: false]}, {:telemetry, "~> 0.4", [hex: :telemetry, repo: "hexpm", optional: true]}], "hexpm", "b5cd52259817eb8a31f2454912ba1cff4990bca7811918878091cb2ab9e52cb8"},

"plug_crypto": {:hex, :plug_crypto, "1.1.2", "bdd187572cc26dbd95b87136290425f2b580a116d3fb1f564216918c9730d227", [:mix], [], "hexpm", "6b8b608f895b6ffcfad49c37c7883e8df98ae19c6a28113b02aa1e9c5b22d6b5"},
"telemetry": {:hex, :telemetry, "0.4.1", "ae2718484892448a24470e6aa341bc847c3277bfb8d4e9289f7474d752c09c7f", [:rebar3], [], "hexpm", "4738382e36a0a9a2b6e25d67c960e40e1a2c95560b9f936d8e29de8cd858480f"},
}
1 change: 0 additions & 1 deletion test/opentelemetry_plug_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ defmodule MyRouter do
use Plug.Router

plug :match
plug Plug.Telemetry, event_prefix: [:my, :plug]
plug :dispatch

forward "/hello/:foo", to: MyPlug
Expand Down