Skip to content

Conversation

derekkraan
Copy link
Contributor

Hi all,

We have been using this in prod for a good number of months now. This has helped us catch a number of issues, since if a process dies before its span can be closed, the span is simply lost.

I thought about how best to get this into opentelemetry. Not 100% sure this is the correct place, but I think this might be a good way to start the discussion, and it's a lower barrier to getting something into the contrib library than into the core library itself.

If there is the feeling that this doesn't belong here then I will release it as a standalone library.

Looking forward to any feedback on this PR.

@bryannaegele
Copy link
Contributor

Thanks Derek! I think we actually want this in the sdk which means it also needs to be in erlang. Any interest in taking this up?

open-telemetry/opentelemetry-erlang#84

@tsloughter
Copy link
Member

Yes, would like this in SDK. Do you have interest/time in converting to Erlang? If not there may be someone else who is and we should ask in Slack.

@derekkraan
Copy link
Contributor Author

I think it should be manageable. I'll close this PR and open a new one in the otel-erlang repo once I have converted to Erlang (hopefully next week sometime).

@hkrutzer
Copy link

hkrutzer commented Mar 6, 2023

@derekkraan I was integrating this in my codebase and I was wondering if there was a specific reason for doing :ets.insert(__MODULE__, {self(), span_ctx}) inside monitor() instead of handle_call({:monitor, pid})?

@derekkraan
Copy link
Contributor Author

derekkraan commented Mar 6, 2023

@hkrutzer the reason is: it's faster. GenServers are by nature single-threaded.

@hkrutzer
Copy link

hkrutzer commented Mar 6, 2023

Sure, but aren't you waiting to the handle_call to complete anyway? So the throughput of the single Genserver is still the bottleneck in this situation. I suppose that throughput could increase by having the ETS call be performed outside of the Genserver 🙂

@derekkraan
Copy link
Contributor Author

Yes, I meant throughput.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants