Provide a way to configure the tracer using properties file by labbati · Pull Request #862 · DataDog/dd-trace-java

labbati · 2019-06-03T10:31:13Z

So far we only allowed configuration of the Java tracer from system properties and environment variables. This can be a pain for users, especially because a file is easier to track and some values can be very large an not easy to handle in envs or system properties.

This PR introduces a third source of configuration, through a properties file in the format:

dd.service.name=my_service
dd.something.else=value

The path to such file is provided to the runtime as a system property -Ddd.trace.config=/path/to/dd-java-tracer.properties.

mar-kolya · 2019-06-03T13:13:03Z

dd-trace-api/src/main/java/datadog/trace/api/Config.java

There is a comment on top of Config default constructor - you may want to update it to reflect the new reality.

mar-kolya · 2019-06-03T13:15:51Z

dd-trace-api/src/main/java/datadog/trace/api/Config.java

What is the point of running this synchronized ?

My mistake, originally this method was also setting the static variable, while now it only return the result. So no point in doing it synchronized

labbati · 2019-06-03T13:26:27Z

dd-trace-api/src/main/java/datadog/trace/api/Config.java

A comment here. I set a static variable in a constructor which is clearly a smell. The motivation behind this was testability and this is not my favorite approach.

What I would prefer to do is to change the class quite a few moving from imperative static loading of properties to a more instance centric approach, where I define inner providers [systemPropertyProvider, environmentVariableProvider, configurationFileProvider, defaultValuesProvider] instantiated in constructor which are tested in sequence until I find a non null.

But the approach above (that I would be more than happy to implement) requires quite a few changes and I will do only if we all agree.

@mar-kolya , @tylerbenson

First of all: if you simply remove static from that field nothing changes much :).

I think one possible approach here is to implement a separate way of loading fully fledged config file from all possible sources separately and then also implement merging config objects. There is sort of a very primitive implementation of this already in a form of second constructor for Config - but it is not flexible enough at the moment.

I think I don't follow. Are you suggesting to make the various getSettingFromEnvironment and the likes non-static and to pass along the loaded properties? In this case I would not clearly understand why we are treating properties from properties file differently from system properties and environment variables

Oh, right, getSettingFromEnvironment that is static... And not only that - it is also used outside of Config... in this particular case I think that initializing that in constructor is outright dangerous and confusing and I would suggest funding way of not doing that.

I think first step there might be refactoring all places that use getSettingFromEnvironment directly to use actual config instance. And then this can be made non static and proper config inheriting/overwriting scheme could be implemented.

mar-kolya · 2019-06-03T13:37:33Z

dd-trace-api/src/main/java/datadog/trace/api/Config.java

When I commented about updating comment I meant this one :)

… static istance

labbati · 2019-06-13T16:08:33Z

The blocking PR #863 implementing parts of what we discussed in comments is merged, time for a final review. @tylerbenson @mar-kolya

tylerbenson

Just optional comments...

tylerbenson · 2019-06-13T17:11:26Z

dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy

+
+  def "verify fallback to properties file that does not exist does not crash app"() {
+    setup:
+    System.setProperty(PREFIX + CONFIGURATION_FILE, "src/test/resources/do-not-exist.properties")


You may want to clear this property in cleanup.

dd-trace-api/src/main/java/datadog/trace/api/Config.java

tylerbenson · 2019-06-13T17:17:08Z

dd-trace-api/src/main/java/datadog/trace/api/Config.java

+   * @return The public facing system property name
+   */
+  private static String propertyNameToSystemPropertyName(String setting) {
+    return PREFIX + setting;


👍
Should getMapSettingFromEnvironment use this method too?

…i/config-file

tylerbenson · 2019-06-17T15:04:30Z

dd-trace-api/src/main/java/datadog/trace/api/Config.java

+
+    // Normalizing tilde (~) paths for unix systems
+    configurationFilePath =
+        configurationFilePath.replaceFirst("^~", System.getProperty("user.home"));


I'm surprised... I would think java would handle this automatically.

labbati force-pushed the labbati/config-file branch 2 times, most recently from 0a40d8b to a3f8c16 Compare June 3, 2019 12:26

mar-kolya reviewed Jun 3, 2019

View reviewed changes

labbati commented Jun 3, 2019

View reviewed changes

mar-kolya reviewed Jun 3, 2019

View reviewed changes

labbati marked this pull request as ready for review June 3, 2019 13:42

labbati requested a review from a team as a code owner June 3, 2019 13:42

labbati changed the title ~~Labbati/config file~~ Provide a way to configure the tracer using a properties file Jun 4, 2019

labbati added tag: do not merge Do not merge changes comp: core Tracer core labels Jun 4, 2019

labbati added this to the 0.30.0 milestone Jun 4, 2019

labbati self-assigned this Jun 4, 2019

labbati changed the title ~~Provide a way to configure the tracer using a properties file~~ Provide a way to configure the tracer using properties file Jun 5, 2019

labbati force-pushed the labbati/config-file branch 2 times, most recently from 2b9ab9b to 2c07912 Compare June 5, 2019 16:54

labbati changed the base branch from master to labbati/non-static-config June 6, 2019 15:54

labbati force-pushed the labbati/config-file branch from 2560e5d to dd7a69e Compare June 6, 2019 16:20

labbati added 7 commits June 13, 2019 14:35

Config to fallback to properties file to load settings

6fc69ba

Tests for functionality: Config fallback to properties files

1d57a58

Update comment on Config class to show new functionality

4dd6708

Removing synchronized as the load properties method no longer set the…

076c2b1

… static istance

Fix typos

bfd9680

Add properties file to a comment listing all the source of configuration

da05772

Remove unused imports

12dc7c4

labbati force-pushed the labbati/config-file branch from dd7a69e to 12dc7c4 Compare June 13, 2019 12:35

labbati changed the base branch from labbati/non-static-config to master June 13, 2019 16:06

labbati removed the tag: do not merge Do not merge changes label Jun 13, 2019

tylerbenson approved these changes Jun 13, 2019

View reviewed changes

labbati added 4 commits June 14, 2019 11:50

Remove system properties and envs left-over in Config tests

f578c9c

Be more friendly for config file to unix users using tilde for home

d0b3214

Merge branch 'master' of github.com:DataDog/dd-trace-java into labbat…

a3be0cb

…i/config-file

Format code

d865a1a

tylerbenson approved these changes Jun 17, 2019

View reviewed changes

labbati merged commit 72cad28 into master Jun 17, 2019

tylerbenson deleted the labbati/config-file branch June 17, 2019 16:50

Conversation

labbati commented Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

labbati Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

labbati commented Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tylerbenson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

labbati commented Jun 3, 2019 •

edited

Loading

labbati Jun 3, 2019 •

edited

Loading

labbati commented Jun 13, 2019 •

edited

Loading