inital api impl #4

tsloughter · 2019-07-25T14:09:33Z

Opening as a draft PR to get the conversation started (also related to the convo @garthk started in opencensus-beam/opencensus_elixir#23)

There are currently two tracers here: oc_tracer_sdk and oc_tracer_sdk_dyn`. The former uses the pdict context and ets storage spans directly while the latter can have the context and storage set with configuration.

I'm not married to any of this.

tsloughter · 2019-07-25T14:11:42Z

Related docs:

include/opentelemetry.hrl

hauleth · 2019-07-25T14:34:04Z

src/opentelemetry.erl

+%% Before OTP-20 rand:uniform could not give precision higher than 2^56.
+%% Here we do a compile time check for support of this feature and will
+%% combine multiple calls to rand if on an OTP version older than 20.0
+-ifdef(OTP_RELEASE).


Which versions of OTP will we support?

OTP20 will cover Elixir programmers going back to 1.5 as long as the reason they're using an old Elixir isn't because they need an even older OTP.

Use of the :seq_trace label to track spans will be tricky in OTP20 without hoping that the birthday paradox won't give us collisions: it's limited to 28 bits, giving us a 17% chance of a collision per 10K spans. Might not hurt as long as they expire fast, but it'll be a lot easier in OTP21.

src/opentelemetry_app.erl

hauleth · 2019-07-25T14:38:58Z

src/ot_tracer.erl

+
+-include("opentelemetry.hrl").
+
+-define(tracer, (persistent_term:get({opentelemetry, tracer}, ot_tracer_sdk))).


As there is usage of persistent_term then we support only OTP 21+, which mean that earlier part with fallback for old OTPs.

hauleth · 2019-07-25T14:41:02Z

src/ot_tracer.erl

+
+-include("opentelemetry.hrl").
+
+-define(tracer, (persistent_term:get({opentelemetry, tracer}, ot_tracer_sdk))).


Will there always be only one tracer? I haven't checked Java implementation yet.

That is the default tracer. There will only be one default tracer.

hauleth · 2019-07-25T14:46:41Z

There is heavy use of persistent_term, shouldn't most of that be moved to application configuration?

tsloughter · 2019-07-25T14:50:28Z

I use persistent term because it needs to have as low overhead as possible. I'm even uneasy using persistent term for this as it also still entails a slow function call because of not being a static module name in the code.

We can have it compile to an application config if on earlier OTPs and just document this fact.

hauleth · 2019-07-25T14:55:50Z

I think that it will be a pretty low overhead when comparing to other languages anyway.

garthk · 2019-07-28T22:52:48Z

Quite. We're wrapping spans around calls with 10²–10⁵ms latency at work. No need to worry about per-span overheads in the 10⁻²ms range, e.g. Application.get_env/3 like I once wasted too much time trying to optimise around.

Not having lerned me some Erlang, can I assume from Elixir we can @behaviour :opentelemetry.ot_tracer? If so: nice, you've given us one configurable behaviour from which our users can sync with Logger metadata, rummage through Process.get(:"$callers"), muck about with :seq_trace, whatever, and in any order they like.

garthk · 2019-08-04T06:22:08Z

Re-reading your comment in the intro, and the code: is the idea that the default tracer won't be pluggable, so it can go extra fast, but users will be able to plug in the _dyn version instead, and it'll let them plug in alternative implementations for the explicit ot_ctx behaviour and implicit ot_span_ets behaviour? Feels a little convoluted, so I hope I'm wrong.

garthk · 2019-08-04T06:33:21Z

src/ot_ctx.erl

+%%%-------------------------------------------------------------------------
+-module(ot_ctx).
+
+-callback get(any()) -> any().


Q: if the key will always be ?SPAN_CTX, do we need the Key arguments?

A: it'll provide more or less backward-compatible rummaging through the process dictionary (ot_span_ctx_key vs oc_span_ctx_key) with ot_ctx_pdict, and give people a plausible way of sharing the seq_trace label with ot_ctx_seqtrace.

That said, I bet we could type it as an atom.

Tried writing against this API. Straight away, needed to special-case :oc_span_ctx_key so I knew to try other means than just Process.get/1. Strikes me it'd be simpler for API consumers if ot_ctx didn't take keys, and if the seq_trace implementation relied on a lower level API to accomplish plausible sharing.

Tripped over this trying to wrap up the work above:

Special-casing the span context key to try unreliable but useful ways of recovering it, e.g. wading through $callers, could also cause problems for the context restoration feature of the get_current_ctx … start_span … with_span_ctx … finish_span … with_span_ctx pattern, which would end up putting back a context that wasn't really there.

If our goal is to let users track the current span context however they need to, strikes me the behaviour should have put_span_ctx/1, get_span_ctx/0, and recover_span_ctx/0. I'll try it out and report back.

Nearly forgot Logger.metadata sync et al, which'd need a separate behaviour sync_current_span_ctx/2 called every time we change the current span context or its attributes.

I think the ot_tracer_sdk_dyn will become ot_tracer_sdk. Then ctx will be dynamic like tracer. This should resolve your issues, right?

garthk · 2019-08-04T06:52:17Z

src/ot_ctx.erl

+
+-callback get(any()) -> any().
+-callback with_value(any(), any()) -> ok.
+-callback with_value(any(), any(), fun()) -> ok.


-> any()., surely, as it'll return the result of fun/0?

tsloughter · 2019-08-12T15:31:35Z

Re-reading your comment in the intro, and the code: is the idea that the default tracer won't be pluggable, so it can go extra fast, but users will be able to plug in the _dyn version instead, and it'll let them plug in alternative implementations for the explicit ot_ctx behaviour and implicit ot_span_ets behaviour? Feels a little convoluted, so I hope I'm wrong.

That was the original idea but I think you are right that it is too convoluted.

tsloughter · 2019-08-13T01:37:05Z

Pushed updates. I moved the _dyn tracer to be the main sdk module and thus the default as well.

It also now tracks the current tracer used in the pdict: https://github.com/open-telemetry/opentelemetry-erlang/pull/4/files#diff-98920f865a632de900785a223c5e42ccR53

This way if you create a span with a different tracer from the default it will use that tracer instead of the default when the span is finished (or is updated or a child is created).

tsloughter · 2019-08-13T14:16:16Z

I've marked this is ready for review so we can get it merged in before even more gets added in and it gets more of a pain to follow.

tsloughter · 2019-08-14T14:14:34Z

@hauleth @garthk any objections to me merging this? It doesn't mean anything is set in stone but I think it is a good start and allows for people to work on and send PRs for various features (like the seqtrace context isn't implemented yet and samplers are needed) separately.

hauleth · 2019-08-14T14:17:15Z

src/opentelemetry_sup.erl

                 intensity => 0,
                 period => 1},
-    ChildSpecs = [],
+    ChildSpecs = [#{id => ot_span_ets,
+                    start => {ot_span_ets, start_link, [Opts]}}],


As for sure we will expand this I would create ot_span_sup that would handle just spans and would manage everything related to them. This would simplify opentelemetry_sup in future.

Do you mean having opentelemetry_sup start ot_span_sup which starts ot_span_ets? Do you expect there to be other span related children?

@tsloughter I do not know how we want to implement dispatching spans to the handler(s). Will there be another process like in OC?

Ah, I guess I was considering reporters and such as separate from span management.

There is the sweeper too. Yea, I'll make this change. One sec.

src/ot_ctx.erl

separate context propagation otep

http1.1 support

…er-add-timeout refactor opentelemetry_exporter

tsloughter force-pushed the initial-api branch 2 times, most recently from 0ce5016 to 4f109e3 Compare July 25, 2019 14:16

hauleth reviewed Jul 25, 2019

View reviewed changes

tsloughter force-pushed the initial-api branch from 4f109e3 to a2ff481 Compare July 25, 2019 15:24

garthk reviewed Aug 4, 2019

View reviewed changes

garthk mentioned this pull request Aug 5, 2019

Feature: configurable span tracking opencensus-beam/opencensus_elixir#26

Draft

tsloughter force-pushed the initial-api branch 2 times, most recently from d505e73 to d2d6f7f Compare August 13, 2019 01:31

tsloughter force-pushed the initial-api branch from d2d6f7f to 857a300 Compare August 13, 2019 14:14

tsloughter marked this pull request as ready for review August 13, 2019 14:15

tsloughter changed the title ~~wip: inital api impl~~ inital api impl Aug 14, 2019

hauleth reviewed Aug 14, 2019

View reviewed changes

src/ot_ctx.erl Show resolved Hide resolved

tsloughter force-pushed the initial-api branch from 857a300 to 0fe4f0f Compare August 14, 2019 14:30

wip: inital api impl

d2a1e34

tsloughter force-pushed the initial-api branch from 0fe4f0f to d2a1e34 Compare August 14, 2019 14:36

hauleth approved these changes Aug 14, 2019

View reviewed changes

tsloughter requested a review from hauleth August 14, 2019 21:06

hauleth approved these changes Aug 14, 2019

View reviewed changes

tsloughter merged commit 7a131de into open-telemetry:master Aug 14, 2019

hauleth pushed a commit to hauleth/opentelemetry-erlang that referenced this pull request Sep 21, 2020

Merge pull request open-telemetry#4 from tsloughter/otep-66

cf03475

separate context propagation otep

hauleth pushed a commit to hauleth/opentelemetry-erlang that referenced this pull request Sep 30, 2020

Merge pull request open-telemetry#4 from opentelemetry-beam/http-1.1

9c84447

http1.1 support

SergeTupchiy added a commit to SergeTupchiy/opentelemetry-erlang that referenced this pull request Nov 10, 2023

Merge pull request open-telemetry#4 from SergeTupchiy/refactor-export…

719ad2b

…er-add-timeout refactor opentelemetry_exporter

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inital api impl #4

inital api impl #4

tsloughter commented Jul 25, 2019

tsloughter commented Jul 25, 2019

hauleth Jul 25, 2019

garthk Aug 4, 2019

hauleth Jul 25, 2019

hauleth Jul 25, 2019

tsloughter Jul 25, 2019

hauleth commented Jul 25, 2019

tsloughter commented Jul 25, 2019

hauleth commented Jul 25, 2019

garthk commented Jul 28, 2019

garthk commented Aug 4, 2019

garthk Aug 4, 2019 •

edited

Loading

garthk Aug 4, 2019

garthk Aug 4, 2019

tsloughter Aug 12, 2019

garthk Aug 4, 2019

tsloughter commented Aug 12, 2019

tsloughter commented Aug 13, 2019

tsloughter commented Aug 13, 2019

tsloughter commented Aug 14, 2019

hauleth Aug 14, 2019

tsloughter Aug 14, 2019

hauleth Aug 14, 2019

tsloughter Aug 14, 2019

tsloughter Aug 14, 2019


		-include("opentelemetry.hrl").

		-define(tracer, (persistent_term:get({opentelemetry, tracer}, ot_tracer_sdk))).

inital api impl #4

inital api impl #4

Conversation

tsloughter commented Jul 25, 2019

tsloughter commented Jul 25, 2019

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

hauleth commented Jul 25, 2019

tsloughter commented Jul 25, 2019

hauleth commented Jul 25, 2019

garthk commented Jul 28, 2019

garthk commented Aug 4, 2019

garthk Aug 4, 2019 • edited Loading

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

tsloughter commented Aug 12, 2019

tsloughter commented Aug 13, 2019

tsloughter commented Aug 13, 2019

tsloughter commented Aug 14, 2019

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

garthk Aug 4, 2019 •

edited

Loading