Add source code attributes to Elixir tracer macros #550

Cantido · 2023-03-04T22:12:56Z

This change adds thread & source code attributes spans created with the Elixir API, as described here: https://opentelemetry.io/docs/reference/specification/trace/semantic_conventions/span-general/#general-thread-attributes. This is related to #517, but it does not close it since I wasn't able to do the Erlang part (I don't know Erlang). I can certainly do the Elixir part though 😄.

I tested this in a project with Jaeger, and the attributes were all correct, despite my lack of skill with macros.

tsloughter · 2023-03-05T17:47:20Z

apps/opentelemetry_api/lib/open_telemetry/tracer.ex

+      Map.new()
+      |> Map.update(:attributes, source_attrs, &Map.merge(&1, source_attrs))


Suggested change

Map.new()

|> Map.update(:attributes, source_attrs, &Map.merge(&1, source_attrs))

Map.update(:attributes, source_attrs, source_attrs)

Isn't this the same?

Oh, I misread entirely :)

At least I think... the &1 applies to the &Map.merge/2, right? And no thte pipe?

But I realize I still don't see where the original opt attributes are being merged with.

whoopsie, you're absolutely right, I was toying with that while debugging and removed the original opt attributes.

tsloughter · 2023-03-05T17:52:40Z

Nice!

Could you add a simple test? Could even just extend one of the existing tests in test/otel_tests.exs to check these attributes are included in addition to attributes it passes to a start.

Cantido · 2023-03-05T18:59:46Z

How can I check the attributes set in the current span? I'm having trouble finding an example.

tsloughter · 2023-03-05T19:04:53Z

This test adds attributes and then checks they are in the outputted span https://github.com/open-telemetry/opentelemetry-erlang/blob/main/test/otel_tests.exs#L33-L46 -- could just add to that test that the source code attributes are also there.

Cantido · 2023-03-05T19:10:06Z

Perfect! I didn't realize there were tests at the root of the repo too. I'll have tests ready in a jiffy.

Cantido · 2023-03-05T19:20:41Z

Do I need to do anything special to run the tests locally? When I try to run mix test, I get this:

$ mix test
===> Analyzing applications...
===> Compiling opentelemetry
=WARNING REPORT==== 5-Mar-2023::12:19:44.385057 ===
FORMAT ERROR: "OTLP exporter module `opentelemetry_exporter` not found. Verify you have included the `opentelemetry_exporter` dependency." - [opentelemetry_exporter]
There are no tests to run

Cantido · 2023-03-05T19:22:34Z

I also checked CI, and I ran mix test --no-start test/otel_tests.exs and got another error.

$ mix test --no-start test/otel_tests.exs
===> Analyzing applications...
===> Compiling opentelemetry

== Compilation error in file test/otel_tests.exs ==
** (RuntimeError) error parsing file /home/rosa/workspace/opentelemetry-erlang/_build/test/lib/opentelemetry/src/otel_tracer.hrl, got: {:error, :enoent}
    (elixir 1.14.3) lib/record/extractor.ex:84: Record.Extractor.read_file/2
    (elixir 1.14.3) lib/record/extractor.ex:50: Record.Extractor.extract_record/2
    test/otel_tests.exs:12: (module)
    (elixir 1.14.3) lib/kernel/parallel_compiler.ex:449: Kernel.ParallelCompiler.require_file/2
    (elixir 1.14.3) lib/kernel/parallel_compiler.ex:342: anonymous fn/5 in Kernel.ParallelCompiler.spawn_workers/7

tsloughter · 2023-03-06T01:27:03Z

You may need to first run rebar3 as test compile.

tsloughter · 2023-03-06T18:51:20Z

apps/opentelemetry_api/lib/open_telemetry/tracer.ex

+      code_attrs = unquote(code_attrs)
+      start_opts =
+        Map.new(unquote(start_opts))
+        |> Map.put(unquote(thread_id_key), :erlang.system_info(:scheduler_id))


Isn't this adding the thread id attribute to start_opts instead of the attributes?

Also, is the Map.new needed?

And I'd suggest moving the thread_id to code_attributes() so it doesn't have to be duplicated, and may mean this whole pipeline can be reduced to a Map.update. The fewer map creations/updates the better :)

Good catch! I'll fix that. And I'll see how removing the Map.new turns out.

I moved thread_id out of code_attributes because I was worried that I was getting the compile-time scheduler instead of the runtime one, so I wanted to move it to inside the quote block.

Oh, the __CALLER__ calls work because they are macros but :erlang.system_info(:scheduler_id) is going to be run at compile time?

Sounds right. I get confused with this macro stuff, esp when its macros in macros :)

Yep! I'm trying to wrap my head around macros myself, I don't work with them much.

So I tried removing the Map.new and the tests failed, there was a test that passes start opts as a keyword list. Since opts is user input and this function is dealing with it as a map, I think it's a good defensive move.

Cantido · 2023-03-07T04:00:12Z

I got the tests figured out! Now they're actually, y'know, testing this change. Thank you for the guidance :). This should be all set for another once-over.

tsloughter · 2023-03-21T22:05:17Z

@Cantido hey, sorry, can you add a changelog entry for this and then I'll merge it?

Or actually, I'll go merge your one changing the macros to functions and then you'll want to rebase on that.

Cantido · 2023-03-22T00:37:21Z

Sure thing!

tsloughter · 2023-03-22T16:55:18Z

I'm stalling a little on this because I learned almost no language does this by default.

I'm leaning towards it still being good to do always since it is only part of the macro. Manual calls to the otel_tracer module to start a span will not add them because it can't even if we wanted it to.

tsloughter · 2023-05-13T11:10:45Z

apps/opentelemetry_api/lib/open_telemetry/tracer.ex

+            thread_id_key: thread_id_key
+          ] do
+      code_attrs = Map.put(code_attrs, thread_id_key, :erlang.system_info(:scheduler_id))
+


I know you mentioned not thinking it should be the scheduler id but the pid before. I wonder if we shouldn't just leave out the thread attribute for now. I'd like to get the pid in but not sure what attribute key to use for it yet and don't want to have our hands tied by backwards compatibility if we add this.

@Cantido do you think you'll have time to update this PR to remove this?

I can certainly do that!

tsloughter · 2023-11-01T10:18:15Z

apps/opentelemetry_api/lib/open_telemetry/tracer.ex

    quote do
+      code_attrs =
+        Map.put(unquote(code_attrs), unquote(thread_id_key), :erlang.system_info(:scheduler_id))


Looks like thread_id is still in here?

My bad, I haven't had a chance to properly sit down with this and get any context back. I promise I will get to that soon.

Cantido requested a review from a team March 4, 2023 22:12

github-actions bot added language-elixir scope-api labels Mar 4, 2023

tsloughter reviewed Mar 5, 2023

View reviewed changes

tsloughter reviewed Mar 6, 2023

View reviewed changes

Cantido added 7 commits March 21, 2023 18:36

Add source code attributes to Elixir tracer macros

0f19904

Fix original opts not being added to span

b01b62d

Fix getting the compile time scheduler instead of the runtime scheduler

92ab754

Fix thread_id going in the wrong place

c5a1a2b

Check for code and thread attrs in main test

3033ab6

Assert specific value for thread.id

1cd3445

mix format

a7076e5

Update CHANGELOG

cde2961

Cantido force-pushed the feature/source-code-span-attrs branch from 6d4809c to cde2961 Compare March 22, 2023 00:39

tsloughter reviewed May 13, 2023

View reviewed changes

tsloughter mentioned this pull request Aug 23, 2023

Trace Liveview in Phoenix instrumentation open-telemetry/opentelemetry-erlang-contrib#198

Merged

Cantido added 2 commits October 30, 2023 15:30

Remove scheduler ID from source code attrs

528b84f

Remove the match for that arg too

7967902

tsloughter reviewed Nov 1, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add source code attributes to Elixir tracer macros #550

Add source code attributes to Elixir tracer macros #550

Cantido commented Mar 4, 2023

tsloughter Mar 5, 2023

tsloughter Mar 5, 2023

tsloughter Mar 5, 2023

Cantido Mar 5, 2023

tsloughter commented Mar 5, 2023

Cantido commented Mar 5, 2023

tsloughter commented Mar 5, 2023

Cantido commented Mar 5, 2023

Cantido commented Mar 5, 2023

Cantido commented Mar 5, 2023

tsloughter commented Mar 6, 2023

tsloughter Mar 6, 2023

Cantido Mar 7, 2023

tsloughter Mar 7, 2023

Cantido Mar 7, 2023

Cantido commented Mar 7, 2023

tsloughter commented Mar 21, 2023

Cantido commented Mar 22, 2023

tsloughter commented Mar 22, 2023

tsloughter May 13, 2023

tsloughter Oct 29, 2023

Cantido Oct 29, 2023

tsloughter Nov 1, 2023

Cantido Nov 2, 2023

		Map.new()
		\|> Map.update(:attributes, source_attrs, &Map.merge(&1, source_attrs))

	Map.new()
	\|> Map.update(:attributes, source_attrs, &Map.merge(&1, source_attrs))
	Map.update(:attributes, source_attrs, source_attrs)

Add source code attributes to Elixir tracer macros #550

Are you sure you want to change the base?

Add source code attributes to Elixir tracer macros #550

Conversation

Cantido commented Mar 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsloughter commented Mar 5, 2023

Cantido commented Mar 5, 2023

tsloughter commented Mar 5, 2023

Cantido commented Mar 5, 2023

Cantido commented Mar 5, 2023

Cantido commented Mar 5, 2023

tsloughter commented Mar 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cantido commented Mar 7, 2023

tsloughter commented Mar 21, 2023

Cantido commented Mar 22, 2023

tsloughter commented Mar 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment