DD_SERVICE; DD_ENV; DD_VERSION env vars support by lpriima · Pull Request #1341 · DataDog/dd-trace-java

lpriima · 2020-03-26T09:11:11Z

Introducing support of 3 new java system.properies / environment variables:

dd.service / DD_SERVICE
dd.env / DD_ENV
dd.version / DD_VERSION

Values of all these 3 env vars are added to tags and overwrite corresponding keys in dd.tags(DD_TAGS) which later become part of mergedSpanTags .
If it least one of these 3 env variables is defined, then Config::tags become not empty and java system property dd.trace.global.tags is totally ignored
If these env variables are not defined: no changes in existing logic and no new fields, tags added to spans and no new payload in logs will be added.

devinsba

I'm not ready to approve yet. I need this to read a couple more times. Unfortunately the changes that aren't needed for these 3 environment variables make reviewing this more difficult

randomanderson

A few small issues I commented on

randomanderson · 2020-04-01T22:45:35Z

-  private static <V extends Enum<V>> Set<V> convertStringSetToEnumSet(
-      final Set<String> input, final Class<V> clazz) {
+  @NonNull
+  private static Set<PropagationStyle> convertStringSetToEnumSet(final Set<String> input) {


While I don't necessarily agree with removing the generic enum handling, I can't argue against it because we haven't needed another enum setting 🤷‍♂

However, with the change, the current method names of:

convertStringSetToEnumSet

getEnumSetSettingFromEnvironmentOrDefault

getPropertySetValue

are confusing/inaccurate

My point is we don't need generalisation(generic) if we don't use it for any other type argument more than a year (or some other significant timeline).

All these 3 method names were confusing even before this change, because they were using LinkedHashSet (iterates over elements in the same order they were added to Map) inside and never used actual EnumSet ( https://docs.oracle.com/javase/7/docs/api/java/util/EnumSet.html ) class where iteration order of entries is always the same according to declaration values in Enum. Anyway, it's a good point and I've renamed these 3 methods.

Given the generic nature of Config, I'd probably vote to leave it, but I don't feel strongly.

On the whole, I find Config both too specific and too generic at the same time.
I really like Config to handle String to primitive conversions better -- especially for List, Maps, but long term, I'd like the details of the precise variables moved elsewhere.

I also find adding a new variable tedious but more importantly error prone. I think a new design could fix that, but I think we should talk through the design first.

tylerbenson

Can this be split up into separate commits/PRs? It seems there are some extra changes that may not make sense.

mar-kolya · 2020-04-06T15:11:37Z

FWIW I think that this needs to be split into two PRs: one with new vars and another one with refactoring changes. And preferably latter having some explanation of what the goal and/or new design is. Otherwise it is really hard to track and understand.

dougqh · 2020-04-10T15:54:39Z

ASIDE: I actually feel that the better opportunity to clean-up Config lies in these various static helpers.

Right now, we sprinkle propertyNameToSystemPropertyName, etc around the code, but I think that could be encapsulated. If we made a Config.Lookup helper class, then we could have a few variations...

PropertiesLookup, EnvLookup -- those could handle applying the appropriate name transformation.
Additionally, we could have a CompositeLookup that handles look first at Env then Properties then Parent.

That said, I don't want to do that as part of this PR for a couple reasons.

1 - I'd like to separate bigger refactorings / non-functional changes from functional changes.
It looks like we separated the commits (which is good), but at certain size, I think we should separate the PRs as well. Admittedly, we need to all be better about doing such a separation -- and this PR isn't quite so big that I find it problematic, but I think Config needs a more serious overall and that should be done by itself.

2 - Kind of the same point as above, but I'd like to get the functional change released sooner rather than later. And right now, we're debating largely non-functional pieces, I don't want to add to that the problem by changing Config enen more.

dougqh · 2020-04-10T16:48:13Z

Isn't this more or less functionally equivalent to enumSet support that came before? Just not public?

I appreciate the use of MethodHandles. I think we've actually been using reflection in places where we should really be using MethodHandles; however, I'm not sure this is one of those places.

I think just looping over the enum values and checking name is probably a little more intuitive and probably faster, too. (Although, this code shouldn't be performance critical.)
Also, I don't really like exposing a name in the code directly out to the config.
I think this can lead to subtle & surprising breaking changes, but that's a problem that already existed.

Isn't this more or less functionally equivalent to enumSet support that came before? Just not public?

No. It's not related to "enumSet support that came before". We never used the java.util.EnumSet class here. We only used "enumSet" string in function names, which created confusion and was potentially dangerous before. And it's discussed here: #1341 (comment)

This valueOf method is used for parsing Integer/Boolean/Float/Double:

https://github.com/DataDog/dd-trace-java/pull/1341/files#diff-43038f0f76f33bbf93ca377b0f77ff7aR954
https://github.com/DataDog/dd-trace-java/pull/1341/files#diff-43038f0f76f33bbf93ca377b0f77ff7aR963
https://github.com/DataDog/dd-trace-java/pull/1341/files#diff-43038f0f76f33bbf93ca377b0f77ff7aR974
https://github.com/DataDog/dd-trace-java/pull/1341/files#diff-43038f0f76f33bbf93ca377b0f77ff7aR982
https://github.com/DataDog/dd-trace-java/pull/1341/files#diff-43038f0f76f33bbf93ca377b0f77ff7aR988

https://github.com/DataDog/dd-trace-java/pull/1341/files#diff-43038f0f76f33bbf93ca377b0f77ff7aR1095
https://github.com/DataDog/dd-trace-java/pull/1341/files#diff-43038f0f76f33bbf93ca377b0f77ff7aR1100
https://github.com/DataDog/dd-trace-java/pull/1341/files#diff-43038f0f76f33bbf93ca377b0f77ff7aR1105

dougqh

As I've said in my comments, I think we all agree that we've reached the point where we really need to clean-up Config. I think we need to tackle that in another PR with some discussion first.

That said, I think we're mostly in a good place now. The one part that I'd really like to see reverted is the change to the Config constructor.

lpriima · 2020-04-10T17:16:22Z

The one part that I'd really like to see reverted is the change to the Config constructor.

done

test

tylerbenson

This looks much better. Thanks for the cleanup.

tylerbenson · 2020-04-10T18:11:35Z

+    serviceName =
+        getSettingFromEnvironment(
+            SERVICE_NAME, getSettingFromEnvironment(SERVICE, DEFAULT_SERVICE_NAME));


from here it looks like the old SERVICE_NAME will take precedence over the new SERVICE, is that the intention?

lpriima requested a review from a team as a code owner March 26, 2020 09:11

lpriima force-pushed the lpriima/DD_ENV_VARS_support branch 8 times, most recently from 44bbae6 to a547030 Compare March 27, 2020 06:44

devinsba reviewed Mar 30, 2020

View reviewed changes

randomanderson reviewed Apr 1, 2020

View reviewed changes

tylerbenson previously requested changes Apr 2, 2020

View reviewed changes

Comment thread dd-trace-api/src/main/java/datadog/trace/api/Config.java Outdated

lpriima requested review from devinsba and randomanderson April 3, 2020 00:15

lpriima force-pushed the lpriima/DD_ENV_VARS_support branch 2 times, most recently from 926365b to 097dc6f Compare April 3, 2020 20:29

lpriima requested a review from tylerbenson April 3, 2020 22:00

mar-kolya reviewed Apr 6, 2020

View reviewed changes

Comment thread dd-trace-api/src/main/java/datadog/trace/api/Config.java Outdated

lpriima force-pushed the lpriima/DD_ENV_VARS_support branch from 097dc6f to 6e88ed3 Compare April 7, 2020 00:13

tylerbenson reviewed Apr 7, 2020

View reviewed changes

Comment thread dd-trace-api/src/main/java/datadog/trace/api/Config.java Outdated

Comment thread dd-trace-ot/src/main/java/datadog/opentracing/DDSpanContext.java

lpriima force-pushed the lpriima/DD_ENV_VARS_support branch 2 times, most recently from 221ebca to 81777f3 Compare April 8, 2020 22:32

dougqh reviewed Apr 10, 2020

View reviewed changes

Comment thread dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/ConfigUtils.groovy Outdated

dougqh reviewed Apr 10, 2020

View reviewed changes

Comment thread dd-trace-api/src/main/java/datadog/trace/api/Config.java Outdated

dougqh reviewed Apr 10, 2020

View reviewed changes

Comment thread dd-trace-api/src/main/java/datadog/trace/api/Config.java Outdated

dougqh reviewed Apr 10, 2020

View reviewed changes

Comment thread dd-trace-api/src/main/java/datadog/trace/api/Config.java Outdated

dougqh reviewed Apr 10, 2020

View reviewed changes

Comment thread dd-trace-api/src/main/java/datadog/trace/api/Config.java Outdated

DD_SERVICE; DD_ENV; DD_VERSION env vars support

ffdaefb

lpriima added 3 commits April 10, 2020 09:39

Config.valueOf("", null, "") with throw NPE

bcc745f

mark unused public fields @deprecated in Config.java

21d5d03

keep unused public fields untouched

6911c0a

dougqh reviewed Apr 10, 2020

View reviewed changes

retrun 2arg Config ctor

12b5b0a

lpriima force-pushed the lpriima/DD_ENV_VARS_support branch from 81777f3 to 12b5b0a Compare April 10, 2020 16:50

dougqh reviewed Apr 10, 2020

View reviewed changes

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

lpriima requested a review from dougqh April 10, 2020 17:21

dougqh approved these changes Apr 10, 2020

View reviewed changes

lpriima requested a review from tylerbenson April 10, 2020 17:32

tylerbenson approved these changes Apr 10, 2020

View reviewed changes

lpriima merged commit 7f146d7 into master Apr 10, 2020

lpriima deleted the lpriima/DD_ENV_VARS_support branch April 10, 2020 17:50

tylerbenson added this to the 0.48.0 milestone Apr 10, 2020

tylerbenson reviewed Apr 10, 2020

View reviewed changes

andrewsouthard1 mentioned this pull request Apr 15, 2020

Updated env vars, added Play 2.3 support DataDog/documentation#7173

Merged

Conversation

lpriima commented Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devinsba left a comment

Choose a reason for hiding this comment

Uh oh!

randomanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

randomanderson Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

lpriima Apr 2, 2020 • 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!

dougqh Apr 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tylerbenson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mar-kolya commented Apr 6, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dougqh Apr 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dougqh Apr 10, 2020

Choose a reason for hiding this comment

Uh oh!

dougqh Apr 10, 2020 • 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!

lpriima Apr 10, 2020 • 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!

dougqh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lpriima commented Apr 10, 2020

Uh oh!

tylerbenson left a comment

Choose a reason for hiding this comment

Uh oh!

tylerbenson Apr 10, 2020

Choose a reason for hiding this comment

Uh oh!

lpriima Apr 10, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lpriima commented Mar 26, 2020 •

edited

Loading

lpriima Apr 2, 2020 •

edited

Loading

dougqh Apr 10, 2020 •

edited

Loading

lpriima Apr 10, 2020 •

edited

Loading