support for encoding to oc protos and exporting to agent #1
Conversation
| - rebar3/compile | ||
| - rebar3/ct: | ||
| requires: | ||
| - rebar3/compile |
There was a problem hiding this comment.
I think we should also check if the generated proto modules are up to date.
src/oc_reporter_service.erl
Outdated
|
|
||
| init(_Opts) -> | ||
| %% in case this is called before the app has booted | ||
| application:ensure_all_started(opencensus_service). |
There was a problem hiding this comment.
Shouldn't we check on return value here?
There was a problem hiding this comment.
Probably not necessary. It'll be started by the release and fail there and be handled.
There was a problem hiding this comment.
Then I would not try to launch it again in init/1. instead just let it fail if someone didn't configured it properly.
There was a problem hiding this comment.
Eh, why make it more complicated and potentially confusing. If there was a builtin way to set these sorts of dynamic dependencies I'd agree.
| trace_id=30880772589364415633133741367674805915, | ||
| span_id=8257235887736323202, | ||
| start_time={-576460693321990995, 2091207418848923700}, | ||
| end_time={-576460679697900156, 2091207418848923700}}, |
There was a problem hiding this comment.
Maybe it would be worth to property test this exporter?
There was a problem hiding this comment.
Yea, probably all of them could benefit from that.
| load_module => #{module => trunc_string(Module) | ||
| %% build_id => undefined | ||
| } | ||
| %% source_version => undefined |
There was a problem hiding this comment.
Testing leftovers or unimplemented features?
There was a problem hiding this comment.
Unimplemented. Not sure what they should actually be yet.
There was a problem hiding this comment.
I guess build_id could be the beam module checksum... source_version would end up being the same I think, since rare that anyone puts a vsn attribute in their modules.
There was a problem hiding this comment.
I would go with application:get_key(App, vsn) instead of module checksum, at least for source_version.
There was a problem hiding this comment.
Hm, yea, I was thinking it meant the module version, now I'm not sure.
And we'd need to track what application a module is in, I don't think there is an efficient way to check that, maybe there is.
There was a problem hiding this comment.
Even if so, the version should be bound to user root application, not to application that issued the span. Otherwise we would need to spawn one OpenCensus instance per application to track all relations between applications as well. Remember that Erlang application is something different from the "application" used by all other OpenCensus libraries.
There was a problem hiding this comment.
The source version here should be for whatever source the frame of the stacktrace relates to. If it was meant to be a source version that relates to a root application it wouldn't be in each frame.
There was a problem hiding this comment.
AFAIK there is no way to know which application given module belongs to. If there is a way, then we could utilise that. Alternative solution is to allow setting that value in the config to release version, because this is what I believe was the "original meaning" in OpenCensus specification.
There was a problem hiding this comment.
Right, that is what I said earlier. Lets just leave them undefined for the time being, the docs on the proto can barely even describe what they are.
There was a problem hiding this comment.
@tsloughter if we would add application:get_application/0 return value to the metadata by default (and this would need to be done by default) then we could handle that.
|
I've updated the grpcbox dep so think this is ready to go except for a few outstanding review discussions. |
|
@hauleth @deadtrickster this ok to merge? Can always make changes on top of it later. |
This needs to wait for a new grpcbox which I'll cut once a new gpb is released. But the actual code for review is ready for reviewing.