more-flexible-kamon-config: adding back a method to allow passing of … #403

Merged
merged 5 commits into from Dec 11, 2016

Projects

None yet

4 participants

@hunterpayne-ck
Contributor

adding back a method to allow passing of a configuration into kamon but still allows for kamon's internal config to control the actor system responsible for metrics

@TomDeMille

why did they remove this in the first place? Makes our configuration painfully awkward without.

@dpsoft
Contributor
dpsoft commented Oct 26, 2016

@TomDeMille take a look #395

@hunterpayne-ck
Contributor

I think it has to do with Kamon needing an Akka ActorSystem. In 0.5.2 and
before Kamon reused an existing ActorSystem (I think) configured in the
standard way (the akka config object). Kamon now configures its own actor
system and configures it from a nested configuration object in a Kamon
specific place in the config. This is to prevent users from accidentally
mis-configuring Kamon's ActorSystem.

This PR is about restoring the ability to pass a config to Kamon without
re-introducing the issues that caused the removal in the first place. In
this case, Kamon still configures its own actor system from the same place
in the config but can use a provided Config instead of one from
ConfigFactory.load() if the user wishes this behavior.

Hunter

On Wed, Oct 26, 2016 at 12:51 PM, Diego Parra notifications@github.com
wrote:

@TomDeMille https://github.com/TomDeMille take a look #395
#395


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#403 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ATKi9cMvQcWXXh53MFyWudOgmpNhDRLfks5q369EgaJpZM4KPUft
.

@rstradling rstradling pushed a commit to BoomTownROI/Kamon that referenced this pull request Oct 28, 2016
@ryanstradling ryanstradling A fork of Kamon for boomtown
This version was forked so that we could release a version that allows the passing in of a config.
It is based upon the PR kamon-io#403 that we believe will get merge back
0ce8a04
@hunterpayne-ck
Contributor

So this PR is ready to go now. Any thing else I need to be before merging?

@hunterpayne-ck
Contributor

This PR is ready to be merged. Diego, can you review and let me know if
there is anything left to do on it or if you have any questions/concerns
about it?

Hunter

On Wed, Oct 26, 2016 at 1:17 PM, Hunter Payne hunter.payne@creditkarma.com
wrote:

I think it has to do with Kamon needing an Akka ActorSystem. In 0.5.2 and
before Kamon reused an existing ActorSystem (I think) configured in the
standard way (the akka config object). Kamon now configures its own actor
system and configures it from a nested configuration object in a Kamon
specific place in the config. This is to prevent users from accidentally
mis-configuring Kamon's ActorSystem.

This PR is about restoring the ability to pass a config to Kamon without
re-introducing the issues that caused the removal in the first place. In
this case, Kamon still configures its own actor system from the same place
in the config but can use a provided Config instead of one from
ConfigFactory.load() if the user wishes this behavior.

Hunter

On Wed, Oct 26, 2016 at 12:51 PM, Diego Parra notifications@github.com
wrote:

@TomDeMille https://github.com/TomDeMille take a look #395
#395


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#403 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ATKi9cMvQcWXXh53MFyWudOgmpNhDRLfks5q369EgaJpZM4KPUft
.

@dpsoft
Contributor
dpsoft commented Nov 16, 2016

@hunterpayne-ck please give me one or two days in order to review this.

@hunterpayne-ck
Contributor

Any progress?

@hunterpayne-ck
Contributor
@dpsoft dpsoft merged commit 1734c2b into kamon-io:master Dec 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dpsoft
Contributor
dpsoft commented Dec 11, 2016 edited

@hunterpayne-ck sorry for the late response, but I've been focusing on scala 2.12 support #410 :(.
In relation to the PR, I think is good for users the possibility to passing the configuration in the Kamon::start method. I will merge this PR and then we need focus in #395.

I just need the @ivantopo ok to be able to merge it in series / *

@hunterpayne-ck
Contributor
@dpsoft dpsoft added this to the 0.6.4 milestone Dec 21, 2016
@ivantopo
Contributor

hey @hunterpayne-ck, I think that there is a bit of a misunderstanding on why the Kamon.start(config) was removed in the first place.. it didn't have anything to do with messing up the user's ActorSystem config with Kamon's internal one, it had to do with the fact that in certain situations (specially, when running tests) you might need to use Kamon without it being started yet because some instrumentation or some code that you can't control is calling Kamon APIs and people would just get a bunch of "Kamon not started yet" errors... it was a pain, and we changed one pain for another, by allowing Kamon to automatically start when needed for the first time and introducing the ConfigProvider so that if you needed to provide some custom config you still could.. but that definitely wasn't the best solution.

For many people this is not a problem, at all, but if you are using Play (and a lot of people do), the experience is terrible.. Kamon stops working after the first application reload (see #395 and #421).. so, even though this PR is bringing back a feature that users want and need (don't get me wrong, we are quite happy that you proposed this! :D), we still need to put a lot of effort to make Kamon behave decently.

I will keep dumping stuff related to this on #395 and #421, as I will be primarily focusing on solving those issues ASAP, so, please stop by and bring as much feedback as you can.

And, once again, thanks for this PR! :D

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