Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stable configuration draft #8328

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
+69 −0
Draft

Stable configuration draft #8328

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5311824
Initial commit
paullegranddc Feb 3, 2025
962cc65
Merge branch 'master' into stable_configuration/add_java_config_source
mtoffl01 Feb 4, 2025
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
2 changes: 2 additions & 0 deletions internal-api/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,8 @@ dependencies {
// it contains annotations that are also present in the instrumented application classes
api "com.datadoghq:dd-javac-plugin-client:0.2.2"

implementation 'org.yaml:snakeyaml:2.0'

testImplementation project(":utils:test-utils")
testImplementation("org.assertj:assertj-core:3.20.2")
testImplementation libs.bundles.junit5
Expand Down
4 changes: 4 additions & 0 deletions internal-api/src/main/java/datadog/trace/api/ConfigOrigin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ public enum ConfigOrigin {
REMOTE("remote_config"),
/** configurations that are set through JVM properties */
JVM_PROP("jvm_prop"),
/** configuration read in the stable config file, managed by customers */
CUSTOMER_STABLE_CONFIG("customer_stable_config"),
/** configuration read in the stable config file, managed by fleet */
FLEET_STABLE_CONFIG("fleet_stable_config"),
/** set when the user has not set any configuration for the key (defaults to a value) */
DEFAULT("default");

Expand Down
63 changes: 63 additions & 0 deletions internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package datadog.trace.bootstrap.config.provider;

import java.io.File;
import java.io.FileInputStream;
import java.io.InputStream;
import java.util.HashMap;

import static datadog.trace.util.Strings.propertyNameToEnvironmentVariableName;

import datadog.trace.api.ConfigOrigin;
import datadog.trace.bootstrap.config.provider.ConfigProvider;

import org.yaml.snakeyaml.Yaml;
import org.yaml.snakeyaml.LoaderOptions;
import org.yaml.snakeyaml.constructor.SafeConstructor;

final public class StableConfigSource extends ConfigProvider.Source {
static final String MANAGED_STABLE_CONFIGURATION_PATH = "/etc/datadog-agent/managed/datadog-agent/stable/datadog_apm.yaml";
static final String LOCAL_STABLE_CONFIGURATION_PATH = "/etc/datadog-agent/datadog_apm.yaml";

final ConfigOrigin fileOrigin;
HashMap<String, String> configuration;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚪ Code Quality Violation

Suggested change
HashMap<String, String> configuration;
Map<String, String> configuration;
Avoid using a specific implementation type; use the more general Map instead (...read more)

Relying on particular implementation types, such as, HashSet or LinkedList can limit your adaptability to embrace alternative implementations in the future, particularly as your requirements change and your code needs to undergo changes.

It is recommended to opt for general types such as Set or List when declaring variables and parameters.

View in Datadog  Leave us feedback  Documentation


StableConfigSource(String file, ConfigOrigin origin) {
this.fileOrigin = origin;
try {
configuration = parseStableConfig(file);
} catch (Exception e) {
configuration = new HashMap();
Copy link
Contributor

Choose a reason for hiding this comment

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

You should still be able to declare configuration as final, javac is smart enough to figure out only a single assignment happens here. For an empty map (if you change the declared type to Map instead of HashMap) you can also use Map.of() or Collections.emptyMap(). Finally, we should at least report the exception if the file exists but is not readable/parseable.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

}
}
Comment on lines +24 to +31
Copy link
Contributor

Choose a reason for hiding this comment

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

⚪ Code Quality Violation

Consider adding super() or this() to your constructor (...read more)

In Java, it is suggested to call super() in an extended class. This rule will report a violation if both a call to super() and an overloaded constructor are absent.

View in Datadog  Leave us feedback  Documentation


private static final HashMap<String, String> parseStableConfig(String filePath) {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚪ Code Quality Violation

Suggested change
private static final HashMap<String, String> parseStableConfig(String filePath) {
private static final Map<String, String> parseStableConfig(String filePath) {
Avoid using a specific implementation type; use the more general Map instead (...read more)

Relying on particular implementation types, such as, HashSet or LinkedList can limit your adaptability to embrace alternative implementations in the future, particularly as your requirements change and your code needs to undergo changes.

It is recommended to opt for general types such as Set or List when declaring variables and parameters.

View in Datadog  Leave us feedback  Documentation

Yaml yaml = new Yaml(new SafeConstructor(new LoaderOptions()));
InputStream input = new FileInputStream(new File(filePath));
Copy link
Contributor

Choose a reason for hiding this comment

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

🔵 Code Quality Violation

Do not initialize FileInputStream. Use a Files.newInputStream or Files.newOutputStream instead (...read more)

The classes that creates FileInputStream and FileOutputStream triggers too much garbage collection. Instead, use methods from the nio package that cause less garbage collection.

Learn More

View in Datadog  Leave us feedback  Documentation

Object data = yaml.load(input);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably leaking a file descriptor. You need to make sure to close the InputStream. Usually, the best to do that is using try ( InputStream input = ... ) {
}


HashMap<Sting, Object> config = new HashMap();

return config;
};

public final String get(String key) {
return configuration.get(propertyNameToEnvironmentVariableName(key));
}

public final ConfigOrigin origin() {
return fileOrigin;
}

private class StableConfig {
private String config_id;
private HashMap<String, Object> apm_configuration_default;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚪ Code Quality Violation

Suggested change
private HashMap<String, Object> apm_configuration_default;
private Map<String, Object> apm_configuration_default;
Avoid using a specific implementation type; use the more general Map instead (...read more)

Relying on particular implementation types, such as, HashSet or LinkedList can limit your adaptability to embrace alternative implementations in the future, particularly as your requirements change and your code needs to undergo changes.

It is recommended to opt for general types such as Set or List when declaring variables and parameters.

View in Datadog  Leave us feedback  Documentation

Copy link
Contributor

Choose a reason for hiding this comment

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

Please follow Java naming conventions -- use camel case not underscores


private void setApmConfigurationDefault(HashMap<String, Object> m) {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚪ Code Quality Violation

Suggested change
private void setApmConfigurationDefault(HashMap<String, Object> m) {
private void setApmConfigurationDefault(Map<String, Object> m) {
Avoid using a specific implementation type; use the more general Map instead (...read more)

Relying on particular implementation types, such as, HashSet or LinkedList can limit your adaptability to embrace alternative implementations in the future, particularly as your requirements change and your code needs to undergo changes.

It is recommended to opt for general types such as Set or List when declaring variables and parameters.

View in Datadog  Leave us feedback  Documentation

apm_configuration_default = m;
}

private void setConfigId(String i) {
config_id = i;
}
}
}
Loading