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

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

Closed
wants to merge 2 commits into from

Conversation

tsloughter
Copy link
Contributor

Instead of creating the span on Plug.Telemetry events it now uses the plug adapter events and the router dispatch event to update the name to the name of the route.

Depends on this patch to Plug elixir-plug/plug#934 so uses the github repo as the dep until a new release is made.

Comment on lines +30 to 54
: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
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.

&__MODULE__.handle_exception/4,
config)
end

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

{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…

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

Copy link
Contributor

@garthk garthk left a comment

Choose a reason for hiding this comment

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

elixir-plug/plug#934 is shipped in Plug 1.10.1

@@ -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"},

@@ -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"},

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