Migrate reporters to gen_event #136
Conversation
| @@ -0,0 +1,38 @@ | |||
| -module(oc_internal_timer). | |||
|
|
|||
| -callback ping() -> ok. | |||
There was a problem hiding this comment.
This name isn't perfect, any suggestions?
There was a problem hiding this comment.
What do you think about using mfa() instead of callback module?
hauleth
force-pushed
the
use-gen_event
branch
from
6683a93
to
5a67f73
Compare
|
I don't see what this adds. It seems more complex but without gain, removing/replacing reporters can be done just as easily in the existing code. I also don't see reporting as events. Allowing multiple reportes is fine because of Erlang terms simple lists and list handling, but thinking of them as event handlers feels like going down the wrong path and giving the wrong impression to users. Even though I wouldn't want to use it, preferring to do this off the node being traced, I can see benefits from a sort of pipeline to pass spans through, but it should be separate from a reporter which is just wanting to get spans out of the system. I'll have to take a closer look later but that is my initial thoughts. |
|
I understand nomenclatural difference, but my point is that currently we are duplicating work that already has been done. I can extract this to separate library though that will provide |
|
gen_sink? |
|
Name I came up with for the library that will provide “flushing” data in given intervals. I try to prepare example tomorrow.
…--
Łukasz Jan Niemier
Wiadomość napisana przez Tristan Sloughter ***@***.***> w dniu 11.02.2019, o godz. 01:37:
gen_sink?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Right, I don't think we are duplicating work because ours is simply a function call on an interval. It shouldn't even really be a list, but it didn't really add any complexity to have it be one instead of separate sequence module for the rare case of wanting a sequence of reporters. |
|
List and fact that we allow to dynamically add and remove reporters makes testing so much easier. Additionally it makes configuring reporter in runtime possible as you can do in your application setup: oc_traces:add_handler(oc_datadog_reporter, [{api_key, os:getenv("DD_TOKEN")}])And call it a day, with "old way" it is a little bit harder. |
|
@tsloughter additional advantage is that it will provide uniform interface for configuring reporters of metrics and traces. Just this thing is worth of fixes. And TBH I think that in the end it will make the code simpler, as almost the same modules can be reused for the metrics. |
Codecov Report
@@ Coverage Diff @@
## master #136 +/- ##
==========================================
+ Coverage 79.91% 81.74% +1.82%
==========================================
Files 34 36 +2
Lines 722 701 -21
==========================================
- Hits 577 573 -4
+ Misses 145 128 -17
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #136 +/- ##
==========================================
+ Coverage 79.97% 81.85% +1.88%
==========================================
Files 37 35 -2
Lines 814 700 -114
==========================================
- Hits 651 573 -78
+ Misses 163 127 -36
Continue to review full report at Codecov.
|
hauleth
changed the title
Close #133
Close #110