opentelemetry-beam/opentelemetry_plug

OpenTelemetry integration for Plug

opentelemetry_plug's Issues

Crash out of handle_start with Plug.Cowboy

This code in handle_start/4…

    {_, adapter} = conn.adapter
    user_agent = header_or_empty(conn, "User-Agent")
    host = header_or_empty(conn, "Host")
    peer_ip = Map.get(Map.get(adapter, :peer_data), :address)

… can crash out when adapter in the first line has no peer_data, but does have peer: {{127, 0, 0, 1}, 63288}, as seen with Plug.Cowboy 2.2.

11:00:54.719 [error] Handler {OpentelemetryPlug, :plug_tracer_start} has failed and has been detached. Class=:error
Reason={:badmap, nil}
Stacktrace=[
  {Map, :get, [nil, :address, nil], 
  [file: 'lib/map.ex', line: 450]}, 
  {OpentelemetryPlug, :handle_start, 4, [file: 'lib/opentelemetry_plug.ex', line: 78]}

The fix seems to be to use c:Plug.Conn.Adapter.get_peer_data/1:

    {adapter_module, payload} = conn.adapter
    peer_data = adapter_module.get_peer_data(payload)

Module and directory naming convention

• Opentelemetry or OpenTelemetry?
• OpenTelemetrySomething or OpenTelemetry.Something? I was initially surprised at the former but I like how it avoids alias clashing with the Something
• opentelemetry/something or open_telemetry/something or open_telemetry_something or opentelemetry_something?

Asking because I've already been inconsistent in my opentelemetry_honeycomb and this project is doing something else again. :)

Remove Phoenix route handling

We can do that in https://github.com/opentelemetry-beam/opentelemetry_phoenix, no?

The x-forwarded-for header is not parsed correctly

The x-forwarded-for header can contain multiple values, where the first value represents the client ip.

Source: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For

I've opened a PR already for this for Phoenix opentelemetry-beam/opentelemetry_phoenix#31, can also do it here if there's interest (probably later this week).

net.peer.* vs net.host.*

lib/opentelemetry_plug.ex reads wrong to me:

    attributes = [{"http.target", conn.request_path},
                  {"http.host",  conn.host},
                  {"http.scheme",  conn.scheme},
                  {"http.user_agent", user_agent},
                  {"http.method", conn.method},
                  {"net.peer.ip", to_string(:inet_parse.ntoa(peer_ip))},
                  {"net.peer.port", adapter.peer_data.port},
                  {"net.peer.name", host},
                  {"net.transport", "IP.TCP"},
                  {"net.host.ip", to_string(:inet_parse.ntoa(conn.remote_ip))},
                  {"net.host.port", conn.port} | optional_attributes(conn)
                  # {"net.host.name", HostName}
                 ]

vs the semantic conventions spec:

Attribute name	Notes and examples
net.transport	Transport protocol used. See note below.
net.peer.ip	Remote address of the peer (dotted decimal for IPv4 or RFC5952 for IPv6)
net.peer.port	Remote port number as an integer. E.g., 80.
net.peer.name	Remote hostname or similar, see note below.
net.host.ip	Like net.peer.ip but for the host IP. Useful in case of a multi-IP host.
net.host.port	Like net.peer.port but for the host port.
net.host.name	Local hostname or similar, see note below.

… and the semantic conventions for HTTP spec, from which I lack the time to extract the right bit, and Plug.Conn:

remote_ip - the IP of the client, example: {151, 236, 219, 228}. This field is meant to be overwritten by plugs that understand e.g. the X-Forwarded-For header or HAProxy's PROXY protocol. It defaults to peer's IP

If conn.remote_ip has the apparent client IP address from X-Forwarded-For, that should go in http.client_ip, which you did by other means, not in net.host.ip. So there'll be some cleaning up in there.

Package not published

Hello,
It appears that this package is not (no longer ?) published on hex.pm, is it normal ? is it ready to be used or still under development ?
cheers

Can't watch Plug adapter telemetry

… because it no longer exists. If we want to kick off a span before we hit the router, I think we're going to need to define a Plug.

adapter telemetry: failure or exception?

Given elixir-plug/plug#944, there's a slight chance we'll need to support Plug adapters calling :telemetry.execute/4 with [:plug_adapter, :call, :exception] eg. plug_cowboy and adapters calling it with [:plug_adapter, :call, :failure] as documented.

It's nothing :telemetry.attach_many/4 won't fix… as long as none of the adapters cope with the ambiguity by calling both.

Custom route filter, header propagation, etc

Splitting this one out of #1…

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

Users might want to customise:

• Which paths are traced and which aren't
• What sample rate to apply, either through a kludge like garthk/opentelemetry_honeycomb#5 or some later API-blessed solution
• How http.client_ip is derived from headers eg. they need RFC 7239 Forwarded as well as X-Forwarded-For
• How the trace context is derived from headers eg. they need to handle Honeycomb beeline as well as traceparent
• Their own attributes derived from the conn
• Other stuff that hasn't occurred to me

Strikes me we could let them do all that with one config item instead of half a dozen:

• Add a custom_start callback option
• Expose our attributes making machinery so they can call it to get most of the heavy lifting done for free

Our code would look something like this:

  # 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)

Not hard, and lets them get as custom as they want:

  # 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