Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

Support plugin configuration via environment variables #192

Merged
merged 1 commit into from
Mar 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 26 additions & 22 deletions src/jaegertracing/ConfigTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "jaegertracing/propagation/HeadersConfig.h"
#include "jaegertracing/samplers/Config.h"
#include "jaegertracing/utils/YAML.h"
#include "jaegertracing/testutils/EnvVariable.h"
#include <gtest/gtest.h>

#include <cstdlib>
Expand Down Expand Up @@ -95,15 +96,6 @@ TEST(Config, testZeroSamplingParam)

#endif // JAEGERTRACING_WITH_YAML_CPP


void setEnv(const char *variable, const char *value) {
#ifdef WIN32
_putenv_s(variable, value);
#else
setenv(variable, value, true);
#endif
}

TEST(Config, testFromEnv)
{
std::vector<Tag> tags;
Expand Down Expand Up @@ -139,20 +131,20 @@ TEST(Config, testFromEnv)
ASSERT_EQ(.7, config.sampler().param());
ASSERT_EQ(std::string("probabilistic"), config.sampler().type());

setEnv("JAEGER_AGENT_HOST", "host33");
setEnv("JAEGER_AGENT_PORT", "45");
setEnv("JAEGER_ENDPOINT", "http://host34:56567");
testutils::EnvVariable::setEnv("JAEGER_AGENT_HOST", "host33");
testutils::EnvVariable::setEnv("JAEGER_AGENT_PORT", "45");
testutils::EnvVariable::setEnv("JAEGER_ENDPOINT", "http://host34:56567");

setEnv("JAEGER_REPORTER_MAX_QUEUE_SIZE", "33");
setEnv("JAEGER_REPORTER_FLUSH_INTERVAL", "45");
setEnv("JAEGER_REPORTER_LOG_SPANS", "true");
testutils::EnvVariable::setEnv("JAEGER_REPORTER_MAX_QUEUE_SIZE", "33");
testutils::EnvVariable::setEnv("JAEGER_REPORTER_FLUSH_INTERVAL", "45");
testutils::EnvVariable::setEnv("JAEGER_REPORTER_LOG_SPANS", "true");

setEnv("JAEGER_SAMPLER_TYPE", "remote");
setEnv("JAEGER_SAMPLER_PARAM", "0.33");
setEnv("JAEGER_SAMPLING_ENDPOINT", "http://myagent:1234");
testutils::EnvVariable::setEnv("JAEGER_SAMPLER_TYPE", "remote");
testutils::EnvVariable::setEnv("JAEGER_SAMPLER_PARAM", "0.33");
testutils::EnvVariable::setEnv("JAEGER_SAMPLING_ENDPOINT", "http://myagent:1234");

setEnv("JAEGER_SERVICE_NAME", "AService");
setEnv("JAEGER_TAGS", "hostname=foobar,my.app.version=4.5.6");
testutils::EnvVariable::setEnv("JAEGER_SERVICE_NAME", "AService");
testutils::EnvVariable::setEnv("JAEGER_TAGS", "hostname=foobar,my.app.version=4.5.6");

config.fromEnv();

Expand All @@ -179,13 +171,25 @@ TEST(Config, testFromEnv)

ASSERT_EQ(false, config.disabled());

setEnv("JAEGER_DISABLED", "TRue"); // case-insensitive
setEnv("JAEGER_AGENT_PORT", "445");
testutils::EnvVariable::setEnv("JAEGER_DISABLED", "TRue"); // case-insensitive
testutils::EnvVariable::setEnv("JAEGER_AGENT_PORT", "445");

config.fromEnv();
ASSERT_EQ(true, config.disabled());
ASSERT_EQ(std::string("host33:445"),
config.reporter().localAgentHostPort());

testutils::EnvVariable::setEnv("JAEGER_AGENT_HOST", "");
testutils::EnvVariable::setEnv("JAEGER_AGENT_PORT", "");
testutils::EnvVariable::setEnv("JAEGER_ENDPOINT", "");
testutils::EnvVariable::setEnv("JAEGER_REPORTER_MAX_QUEUE_SIZE", "");
testutils::EnvVariable::setEnv("JAEGER_REPORTER_FLUSH_INTERVAL", "");
testutils::EnvVariable::setEnv("JAEGER_REPORTER_LOG_SPANS", "");
testutils::EnvVariable::setEnv("JAEGER_SAMPLER_PARAM", "");
testutils::EnvVariable::setEnv("JAEGER_SAMPLER_TYPE", "");
testutils::EnvVariable::setEnv("JAEGER_SERVICE_NAME", "");
testutils::EnvVariable::setEnv("JAEGER_TAGS", "");
testutils::EnvVariable::setEnv("JAEGER_DISABLED", "");
}

} // namespace jaegertracing
5 changes: 3 additions & 2 deletions src/jaegertracing/DynamicLoad.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

*tracerFactory = jaegerTracerFactory;
if (*tracerFactory == nullptr) {
*errorCategory = static_cast<const void*>(&std::generic_category());
return static_cast<int>(std::errc::not_enough_memory);
Expand All @@ -50,4 +51,4 @@ static int makeTracerFactory(const char* opentracingVersion,
return 0;
}

OPENTRACING_DECLARE_IMPL_FACTORY(makeTracerFactory)
OPENTRACING_DECLARE_IMPL_FACTORY(makeTracerFactory)
15 changes: 7 additions & 8 deletions src/jaegertracing/TracerFactory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,19 +38,18 @@ TracerFactory::MakeTracer(const char* configuration,
opentracing::configuration_parse_error);
}

const auto serviceNameNode = yaml["service_name"];
if (!serviceNameNode) {
errorMessage = "`service_name` not provided";
return opentracing::make_unexpected(
opentracing::invalid_configuration_error);
auto tracerConfig = jaegertracing::Config::parse(yaml);

if (_readFromEnv) {
tracerConfig.fromEnv();
}
if (!serviceNameNode.IsScalar()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

errorMessage = "`service_name` must be a string";

if (tracerConfig.serviceName().empty()) {
errorMessage = "`service_name` not provided";
return opentracing::make_unexpected(
opentracing::invalid_configuration_error);
}

const auto tracerConfig = jaegertracing::Config::parse(yaml);
return jaegertracing::Tracer::make(tracerConfig);
#endif // JAEGERTRACING_WITH_YAML_CPP
} catch (const std::bad_alloc&) {
Expand Down
12 changes: 12 additions & 0 deletions src/jaegertracing/TracerFactory.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,18 @@ class TracerFactory : public opentracing::TracerFactory {
opentracing::expected<std::shared_ptr<opentracing::Tracer>>
MakeTracer(const char* configuration, std::string& errorMessage) const
noexcept override;

TracerFactory()
: TracerFactory(false)
{
}

TracerFactory(bool readFromEnv)
: _readFromEnv(readFromEnv)
{
}
private:
bool _readFromEnv;
};

} // namespace jaegertracing
Expand Down
59 changes: 56 additions & 3 deletions src/jaegertracing/TracerFactoryTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@
* limitations under the License.
*/

#include "jaegertracing/Tracer.h"
#include "jaegertracing/TracerFactory.h"
#include "jaegertracing/Constants.h"
#include "jaegertracing/testutils/EnvVariable.h"
#include <gtest/gtest.h>

namespace jaegertracing {
Expand All @@ -27,7 +29,7 @@ TEST(TracerFactory, testInvalidConfig)
R"({
"service_name": {}
})" };
TracerFactory tracerFactory;
TracerFactory tracerFactory(true);
for (auto&& invalidConfig : invalidConfigTestCases) {
std::string errorMessage;
auto tracerMaybe =
Expand Down Expand Up @@ -65,17 +67,68 @@ TEST(TracerFactory, testValidConfig)
"refreshInterval": 60
}
})";
TracerFactory tracerFactory;
TracerFactory tracerFactory(true);
std::string errorMessage;
auto tracerMaybe = tracerFactory.MakeTracer(config, errorMessage);
ASSERT_EQ(errorMessage, "");
ASSERT_TRUE(tracerMaybe);
}

TEST(TracerFactory, testWithoutReadFromEnv)
{
const char* config = "";
testutils::EnvVariable::setEnv("JAEGER_SERVICE_NAME", "AService");
TracerFactory tracerFactory(false);
std::string errorMessage;
auto tracerMaybe = tracerFactory.MakeTracer(config, errorMessage);
ASSERT_FALSE(tracerMaybe);
ASSERT_NE(errorMessage, "");

testutils::EnvVariable::setEnv("JAEGER_SERVICE_NAME", "");
}

TEST(TracerFactory, testWithReadFromEnv)
{
const char* config = "";
testutils::EnvVariable::setEnv("JAEGER_SERVICE_NAME", "AService");
TracerFactory tracerFactory(true);
std::string errorMessage;
auto tracerMaybe = tracerFactory.MakeTracer(config, errorMessage);
ASSERT_EQ(errorMessage, "");
ASSERT_TRUE(tracerMaybe);

auto tracer = tracerMaybe.value();
const auto jaegerTracer = std::dynamic_pointer_cast<jaegertracing::Tracer>(tracer);
ASSERT_EQ(std::string("AService"), jaegerTracer->serviceName());

testutils::EnvVariable::setEnv("JAEGER_SERVICE_NAME", "");
}

TEST(TracerFactory, testEnvTakesPrecedence)
{

const char* config = R"(
{
"service_name": "Ignored"
})";
testutils::EnvVariable::setEnv("JAEGER_SERVICE_NAME", "test");
TracerFactory tracerFactory(true);
std::string errorMessage;
auto tracerMaybe = tracerFactory.MakeTracer(config, errorMessage);
ASSERT_EQ(errorMessage, "");
ASSERT_TRUE(tracerMaybe);

auto tracer = tracerMaybe.value();
const auto jaegerTracer = std::dynamic_pointer_cast<jaegertracing::Tracer>(tracer);
ASSERT_EQ(std::string("test"), jaegerTracer->serviceName());

testutils::EnvVariable::setEnv("JAEGER_SERVICE_NAME", "");
}
#else
TEST(TracerFactory, failsWithoutYAML)
{
const char* config = "";
TracerFactory tracerFactory;
TracerFactory tracerFactory(true);
std::string errorMessage;
auto tracerMaybe = tracerFactory.MakeTracer(config, errorMessage);
ASSERT_NE(errorMessage, "");
Expand Down
17 changes: 17 additions & 0 deletions src/jaegertracing/testutils/EnvVariable.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*
* Copyright (c) 2019 The Jaeger Authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include "jaegertracing/testutils/EnvVariable.h"
38 changes: 38 additions & 0 deletions src/jaegertracing/testutils/EnvVariable.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/*
* Copyright (c) 2019 The Jaeger Authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#ifndef JAEGERTRACING_TESTUTILS_ENVVARIABLE_H
#define JAEGERTRACING_TESTUTILS_ENVVARIABLE_H

#include <string>

namespace jaegertracing {
namespace testutils {
namespace EnvVariable {

inline void setEnv(const char *variable, const char *value) {
#ifdef WIN32
_putenv_s(variable, value);
#else
setenv(variable, value, true);
#endif
}

} // namespace EnvVariable
} // namespace testutils
} // namespace jaegertracing

#endif // JAEGERTRACING_TESTUTILS_ENVVARIABLE_H
2 changes: 1 addition & 1 deletion src/jaegertracing/utils/EnvVariable.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,4 @@ inline std::pair<bool, bool> getBoolVariable(const char* envVar)
} // namespace utils
} // namespace jaegertracing

#endif // JAEGERTRACING_UTILS_HEXPARSING_H
#endif // JAEGERTRACING_UTILS_ENV_VARIABLE_H