separate context propagation otep #4
Conversation
|
Corresponding commit to the SDK open-telemetry/opentelemetry-erlang@21d75b9 |
src/opentelemetry.erl
Outdated
| @@ -106,12 +112,42 @@ | |||
| set_default_tracer(Tracer) -> | |||
| persistent_term:put({?MODULE, default_tracer}, Tracer). | |||
|
|
|||
| set_default_context_manager(ContextModule) -> | |||
| persistent_term:put({?MODULE, context_manager}, ContextModule). | |||
There was a problem hiding this comment.
would it make sense to guard that this is actually a module (or an atom if we don't check for loading)? Like there's a risk of setting a bad value and insta-killing all the things, no?
In fact looking at the call below, the expected form is {Module, Args} and asserting there could prevent huge mistakes.
There was a problem hiding this comment.
Agh yea, very good catch.
There was a problem hiding this comment.
So it is actually used as just a module atom. Clearly need some typespecs so dialyzer will catch this shit where get_* is used.
I'm now unsure if there should be a tuple or not. Depends on if some state (that can't be updated because it is in a persistent term) is needed. I think I have an idea, will update this in a bit.
| end, ok, List), | ||
| ok | ||
| end, | ||
| persistent_term:put({?MODULE, http_extractor}, Fun). |
There was a problem hiding this comment.
This module is no longer safe for hot-code loading, since after two editions of this module being loaded, the Fun will need to be reaped, ending up killing all processes that hold a reference to it?
Though from a quick test, it seems like it just eventually turns into a badfun, so it's not that bad?
I guess the only workaround would be to extract the function to have a kind of "state" argument and use a fully qualified fun to make it work. Probably not worth the effort at this point in time?
| Fun() | ||
| after | ||
| set_current(CtxModule, Namespace, PrevCtx) | ||
| end. |
There was a problem hiding this comment.
I guess this usage implies that the current values for the context can't be global, and only process-global at best? Otherwise running two of these at the same time may break things, right? Might be worth pointing out in a comment?
There was a problem hiding this comment.
Yes, context is tied to a process. I can note that.
| String = "", %% Headers | ||
| New = FromText(String, CtxModule:get_current(Namespace)), | ||
| CtxModule:set_current(Namespace, New) | ||
| end. |
There was a problem hiding this comment.
Ah yea, I'll finish this.
Based on OTEP-66 open-telemetry/oteps#66
Still to do and will be added to this PR before merging: propagators.