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

Merged
merged 8 commits into from
Jun 8, 2020

Conversation

garthk
Copy link
Contributor

@garthk garthk commented May 23, 2020

  • fix compile
  • fix plug dependency
  • fix tests

@garthk
Copy link
Contributor Author

garthk commented May 23, 2020

The test is failing because your handle_route/4 is failing because the span context is :undefined because your handle_start/4 wasn't called because nobody sent a [:plug_adapter, :call, :start] because there's no adapter present.

@garthk
Copy link
Contributor Author

garthk commented May 24, 2020

Ok, I'm going to need to loop around and submit a PR for Plug.Test because it doesn't call into telemetry.execute/3 as required by [reads notes] Plug.Conn.Adapter.

@garthk
Copy link
Contributor Author

garthk commented May 24, 2020

I can make the test pass by calling :telemetry.execute/4 before MyRouter.call/2, but any users who don't run around doing the same to their tests will get a noisy error as our telemetry handler disconnects:

12:41:18.908 [error] Handler {OpentelemetryPlug, :plug_tracer_router_start} has failed and has been detached. Class=:error
Reason=:function_clause
Stacktrace = [
  {:ot_span_ets, :update_name, [:undefined, "/hello/:foo/*glob"],
   [file: '$HOME/opentelemetry_plug/deps/opentelemetry/src/ot_span_ets.erl', line: 131]},
  {:telemetry, :"-execute/3-fun-0-", 4,
   [file: '$HOME/opentelemetry_plug/deps/telemetry/src/telemetry.erl', line: 123]},
  {:lists, :foreach, 2, [file: 'lists.erl', line: 1338]},
  {MyRouter, :dispatch, 2, [file: 'lib/plug/router.ex', line: 277]},
  {MyRouter, :plug_builder_call, 2, [file: 'test/opentelemetry_plug_test.exs', line: 39]},
  {OpentelemetryPlugTest, :"test creates span", 1,
   [file: 'test/opentelemetry_plug_test.exs', line: 9]},
  {ExUnit.Runner, :exec_test, 1, [file: 'lib/ex_unit/runner.ex', line: 355]},
  {:timer, :tc, 1, [file: 'timer.erl', line: 166]},
  {ExUnit.Runner, :"-spawn_test_monitor/4-fun-1-", 4, [file: 'lib/ex_unit/runner.ex', line: 306]}
]

We can compensate for that, having handle_route/4 call handle_start/4 if the context is :undefined, but there's no similarly easy way to get handle_stop/4 called with a finished conn, which brings us back to trying to write advice for our users on how to write a test that'll prove their OT integration works. What do you reckon? This getting messy enough you'd ponder switching back to @behaviour Plug for integration?

@garthk
Copy link
Contributor Author

garthk commented May 24, 2020

Reckon I'll apply José's hint from elixir-plug/plug#945 and take a test-only dependency on plug_cowboy, and have handle_route and handle_stop no-op if the span context is :undefined.

If one of our user's tests don't call through a `Plug.Conn.Adapter` that
calls `:telemetry.execute/4` with a `[:plug_adapter, :call, :start]`
event as expected, our handler will crash out loudly when their router
calls `:telemetry.execute/4` with `[:plug, :router_dispatch, :start]` or
`[:phoenix, :router_dispatch, :start]` if we don't take care to avoid
calling `OpenTelemetry.Span.set_attribute/2` when the span context is
`:undefined`.
@garthk garthk force-pushed the update-name-fix branch from 4fb0aa5 to 1d92075 Compare May 25, 2020 01:43
@garthk garthk changed the base branch from update-name to master May 25, 2020 01:43
@garthk garthk changed the title remove event_prefix from handler identifiers use router dispatch start events to update name of span May 25, 2020
@garthk
Copy link
Contributor Author

garthk commented May 25, 2020

Incorporates #1 for one-click merge convenience.

@garthk garthk mentioned this pull request Jun 4, 2020
garthk and others added 2 commits June 5, 2020 09:57
Make requests through `Plug.Cowboy` to ensure the necessary
`[:plug_adapter, :call, :start]` event, taking necessary dependencies on
`hackney`, `plug`, and `plug_cowboy`. Smells more of an integration test,
but gets the job done.
@garthk garthk force-pushed the update-name-fix branch from 1d92075 to ab2aba1 Compare June 4, 2020 23:57
Comment on lines 138 to 139
# for some reason Elixir removed Enum.filter_map in 1.5
# so just using Erlang's list module
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use Enum.flat_map/2. Is the concern efficiency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kinda doubt it; 𝑛=2. Just another tell that the author comes more from the Erlang side of things? I'd flat map with empty lists instead of false, yeah: this part of the diff is just mix format doing its thing while I edit elsewhere.

@garthk garthk mentioned this pull request Jun 8, 2020
@tsloughter tsloughter merged commit c22a40e into opentelemetry-beam:master Jun 8, 2020
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.

3 participants