merge DD_VERSION and DD_ENV with "dd.trace.global.tags" by lpriima · Pull Request #1393 · DataDog/dd-trace-java

lpriima · 2020-04-21T16:26:15Z

This change introduces merging of explicitly defined span properties and properties defined in "dd.trace.global.tags".

I ) For every property we have 3 levels of defining it:

creating tracer with configuration via API and with explicit Properties argument with specifying this exact property inside this argument ( https://github.com/DataDog/dd-trace-java/blob/master/dd-trace-api/src/main/java/datadog/trace/api/Config.java#L1384 )
java system property
linux environment variable
java property file
We take next one only if it's not defined the previous one. Eg. So we look into linux environment variables only

II ) We have special container properties tags and global.tags which allow to aggregate other properties inside it.

dd.tags(DD_TAGS) has precedence over dd.trace.global.tags. It will be merging of tags.

III ) The result service tag in span is set up by

property(env variable) dd.service (DD_SERVICE) first.
if dd.service (DD_SERVICE) is not set the result service tag is set by dd.service.name (DD_SERVICE_NAME)
if neither of above is set, the result service tag is set by default integration name or just default name
you can't define service tag through tags

tylerbenson

Thanks for all the additional tests.
I wonder if all the tags should be merged into a single map. @mar-kolya what do you think?

tylerbenson · 2020-04-24T14:49:23Z

+    {
+      final Map<String, String> tagsPreMap = getMapSettingFromEnvironment(TAGS, null);
+      if (tagsPreMap != null && !tagsPreMap.isEmpty()) {
+        // we only populate this tags if we use 'dd.tags'. If we don't use it: we populate this tags
+        // to 'dd.trace.global.tags' and globalTags field
+        tags = getMapWithPropertiesDefinedByEnvironment(tagsPreMap, ENV, VERSION);
+      } else {
+        tags = Collections.emptyMap();
+      }
+    }
+    globalTags =
+        getMapWithPropertiesDefinedByEnvironment(
+            getMapSettingFromEnvironment(GLOBAL_TAGS, null), ENV, VERSION);
+


Do you think it would simplify things to reduce the distinction between tags and global tags and combine them into a single map as soon as possible? (End result being just a single tags field.)

+1 I think this idea makes sense too. We just need to be careful about the order of precedence for updating the single map.

E.g.,

Look for DD_ prefixed env var first for env/service/version.

Then look at JMX flags.

tylerbenson · 2020-04-24T14:55:45Z

    then:
-    config.mergedSpanTags == [a: "1", c: "3"]
-    config.mergedJmxTags == [a: "1", d: "4", (RUNTIME_ID_TAG): config.getRuntimeId(), (SERVICE_TAG): config.serviceName]
+    config.mergedSpanTags == [a: "1", c: "3",  (Config.ENV) : "eu-east", (Config.VERSION) : "43"]


When you've implemented merging of the tags/global tags, this should also have b:2, right?

if you want me to merge them. It might break existing backward compatibility because we already have several versions where dd.tags exclude dd.trace.global.tags is it ok ?

yeah, let's merge them into a single map inside Config. Any edge cases where this would be problematic?

yeah, let's merge them into a single map inside Config. Any edge cases where this would be problematic?

@jdgumz ?

The only thing I can think of is a hard-coded service tag existing in some Dockerfile or build config that goes unnoticed. If someone adds DD_SERVICE elsewhere and the value is different, it might be a surprise.

But I think as long as we call out the order of precedence very clearly in our release notes, folks should be able to triage the issue. I don't think we can support differing service values - there has to be one.

jdgumz

Thanks for adding the cases for service. Although I'm not sure I understand the precedence at play.

jdgumz · 2020-04-24T18:15:50Z

+    {
+      final Map<String, String> tagsPreMap = getMapSettingFromEnvironment(TAGS, null);
+      if (tagsPreMap != null && !tagsPreMap.isEmpty()) {
+        // we only populate this tags if we use 'dd.tags'. If we don't use it: we populate this tags
+        // to 'dd.trace.global.tags' and globalTags field
+        tags = getMapWithPropertiesDefinedByEnvironment(tagsPreMap, ENV, VERSION);
+      } else {
+        tags = Collections.emptyMap();
+      }
+    }
+    globalTags =
+        getMapWithPropertiesDefinedByEnvironment(
+            getMapSettingFromEnvironment(GLOBAL_TAGS, null), ENV, VERSION);
+


+1 I think this idea makes sense too. We just need to be careful about the order of precedence for updating the single map.

E.g.,

Look for DD_ prefixed env var first for env/service/version.

Then look at JMX flags.

jdgumz · 2020-04-24T18:18:09Z

+                             'service.version' : 'my-svc-vers']
+  }
+
+  def "DD_SERVICE ignored when 'dd.service.name' java property is set; 'dd.service.name' overwrites DD_SERVICE_NAME"() {


DD_SERVICE should not be ignored - it should have the highest precedence.

In our codebase, we have a different order of precedence. System Properties, defined on the command line have a higher precedence than environment variables.

@tylerbenson here @jdgumz wants java property(env var) dd.service(DD_SERVICE) take precedence over dd.service.name(DD_SERVICE_NAME). That's another potentially backward incompatible change which might affect some deployments where we have both of them already. It looks like previous change where we introduced tags which overwrite old existing global.tags : https://github.com/DataDog/dd-trace-java/pull/1263/files#diff-43038f0f76f33bbf93ca377b0f77ff7aR707

jdgumz · 2020-04-24T18:23:44Z

+
+    then:
+    config.serviceName == "dd-service-env-var"
+    config.mergedSpanTags == [service:'service-tag-in-dd-trace-global-tags-java-property','service.version' : 'my-svc-vers']


I'm not sure I understand. Why are we using the service:service-tag-in-dd-trace-global-tags-java-property still? I think it'd be confusing to see a different service in the span metadata from the one that should be used (the value of DD_SERVICE).

I assume service would get removed from the metadata before being reported to the agent, but I'm wondering if the part that does that would then replace the env-var service name since the rules for processing tags happen at a different place. This might be a problem and mean we need to pull those tags out earlier.

service name will be overwritten for span with the check above

The point of this and some other tests to show: that we can have service tag in tags, which will be later ignored and FOR SURE will be overwritten by some other things up to default service name. That's behaviour is different from env and version tags, which will not be overwritten unless corresponding java property or env var is set.

Gotcha. One other question: it seems like DD_SERVICE overrides the value of service even when there's a service tag in the global system property. This seems inconsistent with dd.service as a system property overriding the env var DD_SERVICE? It sounded like we wanted to maintain the highest precedence for system properties?

we have precedence of

java system properties

environment variables

property file

before, and implications of this might be more unpredictable for many other deployment. Of we want to change this precedence order, let's address it in a separate PR

jdgumz

I think most of the test cases make sense - just have a few more questions.

jdgumz · 2020-04-27T18:07:59Z

+    Config.get().serviceName == DEFAULT_SERVICE_NAME
+  }
+
+  def "default service name is not affected by tags, nor env variables"() {


I'm not sure I understand this case. If we have a service somewhere (global tags, or even DD_SERVICE) shouldn't that be used?

It should not. It will be ignored. That's requirement we have now. You can't define service name through tags. If you try to do so: it will be overwritten later

While this is true from the config perspective, we may have a problem with this when it is actually applied to a span. Spans have decorators that are applied to tags. Specifically ServiceNameDecorator may be problematic. I think we want to update SpanDecoratorTest to validate this.

jdgumz · 2020-04-27T18:20:54Z

+
+    then:
+    config.serviceName == "dd-service-env-var"
+    config.mergedSpanTags == [service:'service-tag-in-dd-trace-global-tags-java-property','service.version' : 'my-svc-vers']


Gotcha. One other question: it seems like DD_SERVICE overrides the value of service even when there's a service tag in the global system property. This seems inconsistent with dd.service as a system property overriding the env var DD_SERVICE? It sounded like we wanted to maintain the highest precedence for system properties?

jdgumz · 2020-04-27T18:25:45Z

+                             'service.version' : 'my-svc-vers']
  }

+  def "DD_SERVICE precedence over 'dd.service.name' java property is set; 'dd.service' overwrites DD_SERVICE"() {


Wait - don't we have it set up to allow DD_ENV and DD_VERSION to override dd.trace.global.tags?

I'm guessing this is a different case because dd.service already existed and generally system properties take the highest precedent?

Wait - don't we have it set up to allow DD_ENV and DD_VERSION to override dd.trace.global.tags?

we don't have to, but if we set these 2 env vars they override corresponding tags. Here is a test for it:
https://github.com/DataDog/dd-trace-java/pull/1393/files#diff-19302d05d6472f1ef9c915823a88f122R1323

I'm guessing this is a different case because dd.service already existed and generally system properties take the highest precedent?

correct

tylerbenson

Approving with the assumption the mentioned edgecase (service tag set overriding the configured service name in an actual span due to decorator interaction) is handled in a separate PR.

lpriima requested review from andrewsouthard1 and tylerbenson April 21, 2020 16:26

lpriima requested a review from a team as a code owner April 21, 2020 16:26

lpriima force-pushed the lpriima/DD_VERSION_tests branch from 1cfd922 to 48402c4 Compare April 21, 2020 21:42

lpriima requested review from dougqh and jdgumz April 21, 2020 21:57

tylerbenson reviewed Apr 21, 2020

View reviewed changes

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

tylerbenson requested a review from mar-kolya April 21, 2020 22:56

lpriima force-pushed the lpriima/DD_VERSION_tests branch from 48402c4 to 5e66734 Compare April 22, 2020 00:10

lpriima changed the title ~~tests set DD_VERSION independently~~ merge DD_VERSION and DD_ENV with "dd.trace.global.tags" Apr 22, 2020

lpriima force-pushed the lpriima/DD_VERSION_tests branch 3 times, most recently from b82171f to baae569 Compare April 22, 2020 07:14

tylerbenson reviewed Apr 24, 2020

View reviewed changes

jdgumz reviewed Apr 24, 2020

View reviewed changes

merge DD_VERSION and DD_ENV with "dd.trace.global.tags"

8994a8b

lpriima force-pushed the lpriima/DD_VERSION_tests branch from baae569 to f50a487 Compare April 27, 2020 06:53

DD_SERVICE priority over DD_SERVICE_NAME; merge TAGS and GLOBAL_TAGS

2bab077

lpriima force-pushed the lpriima/DD_VERSION_tests branch from f50a487 to 2bab077 Compare April 27, 2020 06:55

jdgumz reviewed Apr 27, 2020

View reviewed changes

tylerbenson approved these changes Apr 28, 2020

View reviewed changes

lpriima merged commit 3808bbf into master Apr 28, 2020

lpriima deleted the lpriima/DD_VERSION_tests branch April 28, 2020 15:55

github-actions Bot added this to the 0.50.0 milestone Apr 28, 2020

Conversation

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

Uh oh!

Uh oh!

tylerbenson left a comment • 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!

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!

jdgumz left a comment

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!

Choose a reason for hiding this comment

Uh oh!

lpriima Apr 24, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdgumz left a comment

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!

lpriima Apr 27, 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!

tylerbenson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lpriima commented Apr 21, 2020 •

edited

Loading

tylerbenson left a comment •

edited

Loading

lpriima Apr 24, 2020 •

edited

Loading

lpriima Apr 27, 2020 •

edited

Loading