Support plugin configuration via environment variables #192

johanneswuerbach · 2019-12-02T20:30:49Z

Which problem is this PR solving?

Support using the environment variables added in #181 when using the plugin. E.g. as part of https://github.com/opentracing-contrib/nginx-opentracing/

Short description of the changes

The plugin entry point now tries to parse the yaml file and loads environment variables if they are defined. Once all configuration methods performed, validate that the service name is not empty.

AppVeyorBot · 2019-12-02T20:58:36Z

✅ Build jaeger-client-cpp 53 completed (commit efce16d8c7 by @johanneswuerbach)

yurishkuro · 2019-12-03T00:53:32Z

src/jaegertracing/TracerFactory.cpp

-        "Failed to construct tracer: Jaeger was not build with yaml support.";
-    return opentracing::make_unexpected(
-        std::make_error_code(std::errc::not_supported));
+    auto tracerConfig = jaegertracing::Config();


this feels unintuitive - the configuration is silently ignored in favor of default config? Maybe it should at least have a null check and still error our if not null?

also, does C++ have some standard way to document functions? This one could use it.

this feels unintuitive - the configuration is silently ignored in favor of default config?

I reverted now to the old behaviour of erroring out when no config is provided as it feels out of scope of this PR to change that.

does C++ have some standard way to document functions?

I haven't done C++ in a long time so I would need on your knowledge, but looking at the other files I don't see any documentation.

src/jaegertracing/TracerFactory.cpp

AppVeyorBot · 2019-12-03T07:39:37Z

✅ Build jaeger-client-cpp 60 completed (commit 874a14bc58 by @johanneswuerbach)

src/jaegertracing/testutils/EnvVariable.cpp

src/jaegertracing/testutils/EnvVariable.h

yurishkuro · 2019-12-03T21:32:30Z

src/jaegertracing/TracerFactory.cpp

        errorMessage = "`service_name` not provided";
        return opentracing::make_unexpected(
            opentracing::invalid_configuration_error);
    }
-    if (!serviceNameNode.IsScalar()) {


not sure what this was testing, why is it no longer needed?

This was manually verifying the integrity of the yaml, which should be done by the parsing itself

johanneswuerbach · 2019-12-03T23:25:51Z

@yurishkuro thanks for the feedback, I addressed your comments.

AppVeyorBot · 2019-12-03T23:51:13Z

✅ Build jaeger-client-cpp 61 completed (commit 36e8a5b57d by @johanneswuerbach)

src/jaegertracing/TracerFactoryTest.cpp

yurishkuro · 2019-12-05T21:26:05Z

getting build errors

In file included from /home/travis/.hunter/_Base/29f1068/a9c2d7f/74005d6/Install/include/gtest/gtest.h:1874:0,
                 from /home/travis/build/jaegertracing/jaeger-client-cpp/src/jaegertracing/TracerFactoryTest.cpp:20:
/home/travis/build/jaegertracing/jaeger-client-cpp/src/jaegertracing/TracerFactoryTest.cpp: In member function ‘virtual void jaegertracing::TracerFactory_testConfigAndEnvironment_Test::TestBody()’:
/home/travis/build/jaegertracing/jaeger-client-cpp/src/jaegertracing/TracerFactoryTest.cpp:102:27: error: ‘class opentracing::v2::expected<std::shared_ptr<opentracing::v2::Tracer> >’ has no member named ‘serviceName’
     ASSERT_EQ(tracerMaybe.serviceName(), "test")
                           ^
/home/travis/build/jaegertracing/jaeger-client-cpp/src/jaegertracing/TracerFactoryTest.cpp:102:5: error: template argument 1 is invalid
     ASSERT_EQ(tracerMaybe.serviceName(), "test")
     ^
/home/travis/build/jaegertracing/jaeger-client-cpp/src/jaegertracing/TracerFactoryTest.cpp:102:27: error: ‘class opentracing::v2::expected<std::shared_ptr<opentracing::v2::Tracer> >’ has no member named ‘serviceName’
     ASSERT_EQ(tracerMaybe.serviceName(), "test")
                           ^
/home/travis/build/jaegertracing/jaeger-client-cpp/src/jaegertracing/TracerFactoryTest.cpp:103:1: error: expected ‘;’ before ‘}’ token
 }
 ^
/home/travis/build/jaegertracing/jaeger-client-cpp/src/jaegertracing/TracerTest.cpp: In member function ‘virtual void jaegertracing::Tracer_testTracer_Test::TestBody()’:

AppVeyorBot · 2019-12-05T21:27:26Z

❌ Build jaeger-client-cpp 62 failed (commit bda18a4d74 by @johanneswuerbach)

johanneswuerbach · 2019-12-05T22:20:33Z

@yurishkuro sorry my C++ knowledge is really rusty, any pointers how to properly access the returned tracer instance in the last test to get the service name?

AppVeyorBot · 2019-12-05T22:27:06Z

❌ Build jaeger-client-cpp 63 failed (commit 97658a1323 by @johanneswuerbach)

AppVeyorBot · 2019-12-05T22:53:21Z

❌ Build jaeger-client-cpp 64 failed (commit ac46bdf7fa by @johanneswuerbach)

AppVeyorBot · 2019-12-06T16:42:54Z

❌ Build jaeger-client-cpp 65 failed (commit 54b0de66e0 by @johanneswuerbach)

AppVeyorBot · 2019-12-06T17:17:21Z

❌ Build jaeger-client-cpp 66 failed (commit 01e5ead72f by @johanneswuerbach)

AppVeyorBot · 2019-12-06T17:46:58Z

❌ Build jaeger-client-cpp 67 failed (commit 322395b309 by @johanneswuerbach)

johanneswuerbach · 2019-12-06T18:19:59Z

src/jaegertracing/TracerFactoryTest.cpp

+    ASSERT_TRUE(tracerMaybe);
+
+    auto tracer = tracerMaybe.value();
+    const auto jaegerTracer = std::dynamic_pointer_cast<Tracer>(tracer);


@yurishkuro any hints how I could the jaeger tracer instance here instead of the opentracing::tracer?

The dynamic pointer cast seems not to work, but I don't understand why :-(

not really, but maybe @mdouaihy could suggest something

@mdouaihy I tried:

TEST(TracerFactory, testConfigAndEnvironment) { testutils::EnvVariable::setEnv("JAEGER_SERVICE_NAME", ""); const char* config = R"( { "service_name": "test" })"; testutils::EnvVariable::setEnv("JAEGER_SERVICE_NAME", "AService"); TracerFactory tracerFactory; std::string errorMessage; auto tracerMaybe = tracerFactory.MakeTracer(config, errorMessage); ASSERT_EQ(errorMessage, ""); ASSERT_TRUE(tracerMaybe); const auto tracer = std::static_pointer_cast<jaegertracing::Tracer>(tracerMaybe.value()); ASSERT_EQ(std::string("test"), tracer->serviceName()); }

but this segfaults. Any pointers?

this is failing because the the returned tracer is not a Jaeger Tracer but a NoopTracer.
the reason is the environment variable JAEGER_DISABLED being set to true in ConfigTest.
I think that we should clear the set variables in those tests at the end of the test to avoid polluting other tests executions.

Also note that you'll have to change the configuration of the Sampler and the Reporter to avoid trying to reach a remote address that doesn't exist.

AppVeyorBot · 2019-12-06T18:45:23Z

❌ Build jaeger-client-cpp 68 failed (commit 558eaf66d9 by @johanneswuerbach)

mdouaihy · 2019-12-08T19:00:38Z

Hello,

I find the change a bit unintuitive: the factory is not respecting the configuration the caller provides. Instead, it's reading a global configuration (env variable) instead of just using the configuration provided.

Any reason why not filling the configuration as need before calling the factory?

johanneswuerbach · 2019-12-08T23:38:50Z

Most other jaeger clients are already configurable using environment variables https://www.jaegertracing.io/docs/1.15/client-features/ and we use JAEGER_AGENT_HOST inside kubernetes to inject the agent IP into each pod https://www.jaegertracing.io/docs/1.15/operator/#installing-the-agent-as-daemonset

While the cpp client itself allows to be configured via environment variables, we only use the plugin via
https://github.com/opentracing-contrib/nginx-opentracing/blob/master/README.md.

To inject now the IP we could generate the config, or change the plugin to also respect the common environment variables. I found the second option cleaner and opened this PR, but happy for any other recommendations.

johanneswuerbach · 2019-12-20T21:16:02Z

@mdouaihy, the TracerFactory implements opentracing::TracerFactory so if I understand correctly adding an additional argument wouldn’t be easily possible.
https://github.com/opentracing/opentracing-cpp/blob/4bb431f7728eaf383a07e86f9754a5b67575dab0/include/opentracing/tracer_factory.h#L50-L51

If I understand opentracing/opentracing-cpp#45 (comment) correctly the factory seems also be mainly used for dynamic loading, so I'm not sure modifying only dynamic load would be sufficient?

AppVeyorBot · 2019-12-20T21:36:23Z

✅ Build jaeger-client-cpp 72 completed (commit 465215b007 by @johanneswuerbach)

mdouaihy · 2019-12-21T15:07:28Z

@johanneswuerbach, there is no need to change the makeTracer method. You can add a property to TracerFactory and an argument to the constructor (in DynamicLoad.cpp/makeTracerFactory)

johanneswuerbach · 2019-12-24T19:39:17Z

@mdouaihy is something like e0fd882 what you had in mind?

It looks like environment variable now take always precedent over the file, is that desired or should I add checks to https://github.com/jaegertracing/jaeger-client-cpp/blob/master/src/jaegertracing/Config.cpp#L37 to validate they are only used when nothing is configured yet? The downside is obviously that this would change behaviour of the current, but unreleased Config::fromEnv().

AppVeyorBot · 2019-12-24T19:59:03Z

✅ Build jaeger-client-cpp 73 completed (commit 04b8217493 by @johanneswuerbach)

AppVeyorBot · 2019-12-24T20:25:44Z

✅ Build jaeger-client-cpp 74 completed (commit ae79f85b82 by @johanneswuerbach)

mdouaihy · 2020-01-03T07:29:45Z

Hello @johanneswuerbach, indeed, that's what I had in mind. One small suggestion: maybe we can pass readFromEnv as a constructor parameter (and have a default value for it for backward compatibility). What do you think?

For the precedence of the environment over the config, I dont think we should change the behavior of Config::fromEnv(). Instead, we can define it in TracerFactory::MakeTracer when readFromEnv is defined.

mdouaihy · 2020-03-02T19:19:26Z

Hi @johanneswuerbach,

do you have any news about this PR?

AppVeyorBot · 2020-03-02T22:28:14Z

✅ Build jaeger-client-cpp 115 completed (commit e29e7b5a82 by @johanneswuerbach)

johanneswuerbach · 2020-03-02T23:24:59Z

Sorry for the delay @mdouaihy, I rebased this PR, but are not sure about your suggestions.

Instead, we can define it in TracerFactory::MakeTracer when readFromEnv is defined.

I'm not sure what this means? In this PR I actually prefer the environment variable values now over the config values as fromEnv already defines some kind of a deep merge. What do you think about that?

AppVeyorBot · 2020-03-02T23:50:59Z

✅ Build jaeger-client-cpp 116 completed (commit 28014836c3 by @johanneswuerbach)

src/jaegertracing/TracerFactoryTest.cpp

mdouaihy · 2020-03-03T07:20:20Z

Hi @johanneswuerbach,

Concerning my point Instead, we can define it in TracerFactory::MakeTracer when readFromEnv is defined., what I mean is exactly what you did: In makeTracer, we read the config then override with what we have in the Env Variable.

@yurishkuro, are you confortable with this behavior or shall we reverse it?

Signed-off-by: Johannes Würbach <johannes.wuerbach@googlemail.com>

johanneswuerbach · 2020-03-03T08:33:09Z

@mdouaihy thanks for the pointers, all leaks addressed.

AppVeyorBot · 2020-03-03T09:02:55Z

✅ Build jaeger-client-cpp 117 completed (commit 72e6db93c6 by @johanneswuerbach)

yurishkuro · 2020-03-03T15:13:33Z

src/jaegertracing/DynamicLoad.cpp

@@ -41,7 +41,8 @@ static int makeTracerFactory(const char* opentracingVersion,
        return opentracing::incompatible_library_versions_error.value();
    }

-    *tracerFactory = new (std::nothrow) jaegertracing::TracerFactory{};
+    const auto jaegerTracerFactory = new (std::nothrow) jaegertracing::TracerFactory(true);


I am not a fan of this API. Without looking up the definition, what does ctor(true) mean?

In the other languages we simply provide a fromEnv() initializer. Not only it is explicit, but it also decouples factory initialization and reading env, e.g. it is very common to first initialize the factory with some default configuration, and then allow it to be overwritten by env. Here it looks like env vars are read immediately.

It was originally implemented like that, but @mdouaihy recommended the ctor approach #192 (comment)

I don’t have an opinion and happily change it to your preference

I may be mistaken about the role of TracerFactory. If it's a loading mechanism similar to Java's service loader, then it's probably ok to apply env right in the ctor. While I don't like the bool argument, it's ok for a non-user-facing class.

@yurishkuro, In order to create a tracer, a tracer factory is first created and then MakeTracer method is called to create a tracer.

In MakeTracer, a yaml config file sent as argument. Properties of the tracer are read and a Tracer is configured and built

We would like to read also the env variables. Now since this is a behavioral change, we would like to make this behavior activatable.

The problem is that we cannot change the signature of MakeTracer. it's a public method. the only way is activate this behavior via a parameter passed to the factory at construction time.

johanneswuerbach · 2020-05-29T20:24:27Z

@yurishkuro @mdouaihy would it be possible to get a new official release including this change? That would be amazing 🎉

yurishkuro · 2020-06-08T04:24:56Z

waiting for the build to pass #227

yurishkuro · 2020-06-11T03:36:04Z

released in 0.6.0

…#192) Signed-off-by: Johannes Würbach <johannes.wuerbach@googlemail.com>

yurishkuro reviewed Dec 3, 2019

View reviewed changes

src/jaegertracing/TracerFactory.cpp Outdated Show resolved Hide resolved

johanneswuerbach force-pushed the plugin-from-env branch from 42e2ee2 to d01d182 Compare December 3, 2019 07:13

yurishkuro reviewed Dec 3, 2019

View reviewed changes

johanneswuerbach force-pushed the plugin-from-env branch from d01d182 to 5ec2883 Compare December 3, 2019 23:24

objectiser reviewed Dec 4, 2019

View reviewed changes

src/jaegertracing/TracerFactoryTest.cpp Outdated Show resolved Hide resolved

johanneswuerbach force-pushed the plugin-from-env branch from 5ec2883 to 788f49c Compare December 5, 2019 21:00

johanneswuerbach force-pushed the plugin-from-env branch 2 times, most recently from 5663937 to ca95345 Compare December 5, 2019 22:10

johanneswuerbach force-pushed the plugin-from-env branch 2 times, most recently from 7d78e8b to 86f9fca Compare December 6, 2019 16:40

johanneswuerbach force-pushed the plugin-from-env branch from 86f9fca to 4c6fd45 Compare December 6, 2019 16:47

johanneswuerbach force-pushed the plugin-from-env branch from 4c6fd45 to 510aa96 Compare December 6, 2019 18:18

johanneswuerbach commented Dec 6, 2019

View reviewed changes

johanneswuerbach force-pushed the plugin-from-env branch from 510aa96 to 2c631e2 Compare December 9, 2019 13:58

johanneswuerbach force-pushed the plugin-from-env branch from 2c631e2 to e1c8555 Compare December 20, 2019 21:09

johanneswuerbach force-pushed the plugin-from-env branch from 4853525 to e0fd882 Compare December 24, 2019 19:34

johanneswuerbach force-pushed the plugin-from-env branch from e0fd882 to 363dc6e Compare March 2, 2020 21:50

johanneswuerbach force-pushed the plugin-from-env branch from 363dc6e to dc583ad Compare March 2, 2020 23:23

mdouaihy suggested changes Mar 3, 2020

View reviewed changes

Support plugin configuration via environment variables

43075a2

Signed-off-by: Johannes Würbach <johannes.wuerbach@googlemail.com>

johanneswuerbach force-pushed the plugin-from-env branch from dc583ad to 43075a2 Compare March 3, 2020 08:32

mdouaihy approved these changes Mar 3, 2020

View reviewed changes

yurishkuro reviewed Mar 3, 2020

View reviewed changes

yurishkuro approved these changes Mar 3, 2020

View reviewed changes

yurishkuro merged commit 47fbf19 into jaegertracing:master Mar 3, 2020

Patrick0308 pushed a commit to Patrick0308/jaeger-client-cpp that referenced this pull request Jul 21, 2021

Support plugin configuration via environment variables (jaegertracing…

5a0bed4

…#192) Signed-off-by: Johannes Würbach <johannes.wuerbach@googlemail.com>

lrouquette pushed a commit to lrouquette/jaeger-client-cpp that referenced this pull request Nov 2, 2021

Support plugin configuration via environment variables (jaegertracing…

2d573bd

…#192) Signed-off-by: Johannes Würbach <johannes.wuerbach@googlemail.com>

Support plugin configuration via environment variables #192

Support plugin configuration via environment variables #192

Conversation

johanneswuerbach commented Dec 2, 2019

Which problem is this PR solving?

Short description of the changes

AppVeyorBot commented Dec 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Dec 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanneswuerbach commented Dec 3, 2019

AppVeyorBot commented Dec 3, 2019

yurishkuro commented Dec 5, 2019

AppVeyorBot commented Dec 5, 2019

johanneswuerbach commented Dec 5, 2019

AppVeyorBot commented Dec 5, 2019

AppVeyorBot commented Dec 5, 2019

AppVeyorBot commented Dec 6, 2019

AppVeyorBot commented Dec 6, 2019

AppVeyorBot commented Dec 6, 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

AppVeyorBot commented Dec 6, 2019

mdouaihy commented Dec 8, 2019

johanneswuerbach commented Dec 8, 2019 • edited Loading

johanneswuerbach commented Dec 20, 2019

AppVeyorBot commented Dec 20, 2019

mdouaihy commented Dec 21, 2019

johanneswuerbach commented Dec 24, 2019

AppVeyorBot commented Dec 24, 2019

AppVeyorBot commented Dec 24, 2019

mdouaihy commented Jan 3, 2020

mdouaihy commented Mar 2, 2020

AppVeyorBot commented Mar 2, 2020

johanneswuerbach commented Mar 2, 2020

AppVeyorBot commented Mar 2, 2020

mdouaihy commented Mar 3, 2020

johanneswuerbach commented Mar 3, 2020

AppVeyorBot commented Mar 3, 2020

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

johanneswuerbach commented May 29, 2020

yurishkuro commented Jun 8, 2020

yurishkuro commented Jun 11, 2020

johanneswuerbach commented Dec 8, 2019 •

edited

Loading