Skip to content

Commit eaaea27

Browse files
leoromanovskyleoromanovsky
committed
Address additional agentless source review feedback
1 parent 3b9b97c commit eaaea27

7 files changed

Lines changed: 81 additions & 44 deletions

File tree

‎dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,12 +45,12 @@ public final class ConfigDefaults {
4545
public static final String DEFAULT_SERVLET_ROOT_CONTEXT_SERVICE_NAME = "root-servlet";
4646
public static final String DEFAULT_AGENT_WRITER_TYPE = "DDAgentWriter";
4747
public static final boolean DEFAULT_STARTUP_LOGS_ENABLED = true;
48-
public static final String DEFAULT_FEATURE_FLAGGING_CONFIGURATION_SOURCE = "agentless";
49-
public static final int DEFAULT_FEATURE_FLAGGING_CONFIGURATION_SOURCE_POLL_INTERVAL_SECONDS = 30;
50-
public static final int DEFAULT_FEATURE_FLAGGING_CONFIGURATION_SOURCE_REQUEST_TIMEOUT_SECONDS = 2;
5148

5249
static final boolean DEFAULT_INJECT_DATADOG_ATTRIBUTE = true;
5350
static final String DEFAULT_SITE = "datadoghq.com";
51+
static final String DEFAULT_FEATURE_FLAGGING_CONFIGURATION_SOURCE = "agentless";
52+
static final int DEFAULT_FEATURE_FLAGGING_CONFIGURATION_SOURCE_POLL_INTERVAL_SECONDS = 30;
53+
static final int DEFAULT_FEATURE_FLAGGING_CONFIGURATION_SOURCE_REQUEST_TIMEOUT_SECONDS = 2;
5454

5555
static final boolean DEFAULT_CODE_ORIGIN_FOR_SPANS_INTERFACE_SUPPORT = false;
5656
static final int DEFAULT_CODE_ORIGIN_MAX_USER_FRAMES = 8;

‎internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ import static datadog.trace.api.config.GeneralConfig.TAGS
5757
import static datadog.trace.api.config.GeneralConfig.TRACER_METRICS_IGNORED_RESOURCES
5858
import static datadog.trace.api.config.GeneralConfig.TRACE_OTEL_SEMANTICS_ENABLED
5959
import static datadog.trace.api.config.GeneralConfig.VERSION
60+
import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.FEATURE_FLAGS_CONFIGURATION_SOURCE
6061
import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.FEATURE_FLAGS_CONFIGURATION_SOURCE_AGENTLESS_POLL_INTERVAL_SECONDS
6162
import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.FEATURE_FLAGS_CONFIGURATION_SOURCE_AGENTLESS_REQUEST_TIMEOUT_SECONDS
6263
import static datadog.trace.api.config.JmxFetchConfig.JMX_FETCH_CHECK_PERIOD
@@ -3497,6 +3498,27 @@ class ConfigTest extends DDSpecification {
34973498
config.featureFlaggingConfigurationSourceRequestTimeoutSeconds == 4
34983499
}
34993500
3501+
def "feature flag configuration source normalizes #value to #expected"() {
3502+
setup:
3503+
Properties properties = new Properties()
3504+
if (value != null) {
3505+
properties.setProperty(FEATURE_FLAGS_CONFIGURATION_SOURCE, value)
3506+
}
3507+
3508+
when:
3509+
def config = new Config(ConfigProvider.withPropertiesOverride(properties))
3510+
3511+
then:
3512+
config.featureFlaggingConfigurationSource == expected
3513+
3514+
where:
3515+
value | expected
3516+
null | "agentless"
3517+
"" | "agentless"
3518+
" " | "agentless"
3519+
" ReMoTe_ConFiG " | "remote_config"
3520+
}
3521+
35003522
def "agentless feature flag timing falls back for non-positive values"() {
35013523
setup:
35023524
Properties properties = new Properties()

‎products/feature-flagging/feature-flagging-agent/src/main/java/com/datadog/featureflag/FeatureFlaggingSystem.java‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,9 +47,12 @@ static void initialize(
4747
CONFIG_SERVICE = configService;
4848
EXPOSURE_WRITER = exposureWriter;
4949
} catch (final RuntimeException | Error e) {
50-
exposureWriter.close();
51-
if (configService != null) {
52-
configService.close();
50+
try {
51+
exposureWriter.close();
52+
} finally {
53+
if (configService != null) {
54+
configService.close();
55+
}
5356
}
5457
throw e;
5558
}

‎products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlaggingSystemTest.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,20 @@ void initializationFailureWithoutConfigurationSourceClosesExposureWriter() {
149149
verify(exposureWriter).close();
150150
}
151151

152+
@Test
153+
void initializationFailureClosesConfigurationSourceWhenExposureWriterCloseFails() {
154+
ConfigurationSourceService configService = mock(ConfigurationSourceService.class);
155+
ExposureWriter exposureWriter = mock(ExposureWriter.class);
156+
doThrow(new IllegalStateException("exposure init failed")).when(exposureWriter).init();
157+
doThrow(new IllegalArgumentException("exposure close failed")).when(exposureWriter).close();
158+
159+
assertThrows(
160+
IllegalArgumentException.class,
161+
() -> FeatureFlaggingSystem.initialize(configService, exposureWriter));
162+
163+
verify(configService).close();
164+
}
165+
152166
private static SharedCommunicationObjects sharedCommunicationObjects() {
153167
DDAgentFeaturesDiscovery discovery = mock(DDAgentFeaturesDiscovery.class);
154168
when(discovery.supportsEvpProxy()).thenReturn(true);

‎products/feature-flagging/feature-flagging-lib/build.gradle.kts‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,14 +24,11 @@ dependencies {
2424
api(project(":utils:queue-utils"))
2525

2626
compileOnly(project(":dd-trace-core")) // shading does not work with this one
27-
// Span-enrichment write tier: TraceInterceptor / GlobalTracer / AgentTracer / AgentSpan.
28-
compileOnly(project(":internal-api"))
2927
// Platform JSON writer for the ffe_* tag values.
3028
compileOnly(project(":components:json"))
3129

3230
testImplementation(libs.bundles.junit5)
3331
testImplementation(libs.bundles.mockito)
34-
testImplementation(project(":internal-api"))
3532
testImplementation(project(":utils:test-utils"))
3633
testImplementation(project(":dd-java-agent:testing"))
3734
}

‎products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/AgentlessConfigurationSource.java‎

Lines changed: 16 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import static datadog.communication.http.OkHttpUtils.prepareRequest;
44
import static datadog.trace.util.AgentThreadFactory.AgentThread.FEATURE_FLAG_CONFIGURATION_POLLER;
5+
import static datadog.trace.util.Strings.isBlank;
56

67
import datadog.communication.http.OkHttpUtils;
78
import datadog.logging.RatelimitedLogger;
@@ -11,9 +12,7 @@
1112
import datadog.trace.util.AgentThreadFactory;
1213
import java.io.IOException;
1314
import java.net.HttpURLConnection;
14-
import java.net.URLEncoder;
1515
import java.util.HashMap;
16-
import java.util.Locale;
1716
import java.util.Map;
1817
import java.util.concurrent.Executors;
1918
import java.util.concurrent.ScheduledExecutorService;
@@ -277,26 +276,17 @@ private static boolean isRetryableStatus(final int status) {
277276
}
278277

279278
private void updateEtag(final String nextEtag) {
280-
if (nextEtag != null && !nextEtag.trim().isEmpty()) {
281-
etag = nextEtag;
282-
}
279+
etag = isBlank(nextEtag) ? null : nextEtag;
283280
}
284281

285282
static HttpUrl endpoint(final Config config) {
286283
final String configuredBaseUrl = config.getFeatureFlaggingConfigurationSourceAgentlessBaseUrl();
287-
final String endpoint =
288-
configuredBaseUrl == null
289-
? datadogApiServerDistributionEndpoint(config)
290-
: endpointFromConfiguredBaseUrl(configuredBaseUrl);
291-
final HttpUrl parsed = HttpUrl.parse(endpoint);
292-
if (parsed == null) {
293-
throw new IllegalArgumentException(
294-
"Invalid Feature Flagging HTTP configuration source URL: " + endpoint);
295-
}
296-
return parsed;
284+
return configuredBaseUrl == null
285+
? datadogApiServerDistributionEndpoint(config)
286+
: endpointFromConfiguredBaseUrl(configuredBaseUrl);
297287
}
298288

299-
private static String endpointFromConfiguredBaseUrl(final String configuredBaseUrl) {
289+
private static HttpUrl endpointFromConfiguredBaseUrl(final String configuredBaseUrl) {
300290
final HttpUrl parsed = HttpUrl.parse(configuredBaseUrl.trim());
301291
if (parsed == null) {
302292
throw new IllegalArgumentException(
@@ -306,30 +296,22 @@ private static String endpointFromConfiguredBaseUrl(final String configuredBaseU
306296
return parsed
307297
.newBuilder()
308298
.addPathSegments(DATADOG_API_SERVER_DISTRIBUTION_PATH.substring(1))
309-
.build()
310-
.toString();
299+
.build();
311300
}
312-
return parsed.toString();
301+
return parsed;
313302
}
314303

315-
private static String datadogApiServerDistributionEndpoint(final Config config) {
316-
final StringBuilder endpoint =
317-
new StringBuilder("https://api.")
318-
.append(config.getSite().toLowerCase(Locale.ROOT))
319-
.append(DATADOG_API_SERVER_DISTRIBUTION_PATH);
304+
private static HttpUrl datadogApiServerDistributionEndpoint(final Config config) {
305+
final HttpUrl.Builder endpoint =
306+
new HttpUrl.Builder()
307+
.scheme("https")
308+
.host("api." + config.getSite())
309+
.addPathSegments(DATADOG_API_SERVER_DISTRIBUTION_PATH.substring(1));
320310
final String env = config.getEnv();
321311
if (env != null && !env.isEmpty()) {
322-
endpoint.append("?dd_env=").append(urlEncode(env));
323-
}
324-
return endpoint.toString();
325-
}
326-
327-
private static String urlEncode(final String value) {
328-
try {
329-
return URLEncoder.encode(value, "UTF-8");
330-
} catch (final IOException e) {
331-
throw new IllegalArgumentException("Unable to encode Feature Flagging environment", e);
312+
endpoint.addQueryParameter("dd_env", env);
332313
}
314+
return endpoint.build();
333315
}
334316

335317
static long millis(final int seconds) {

‎products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/AgentlessConfigurationSourceTest.java‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ void derivesDatadogApiServerDistributionEndpointFromSiteAndEnv() {
6666
final Config config = config("datad0g.com", "staging env");
6767

6868
assertEquals(
69-
"https://api.datad0g.com/api/v2/feature-flagging/config/server-distribution?dd_env=staging+env",
69+
"https://api.datad0g.com/api/v2/feature-flagging/config/server-distribution?dd_env=staging%20env",
7070
AgentlessConfigurationSource.endpoint(config).toString());
7171
}
7272

@@ -385,6 +385,25 @@ void ignoresBlankEtag() throws Exception {
385385
verify(listener).accept(any(ServerConfiguration.class));
386386
}
387387

388+
@Test
389+
void successfulResponseWithoutEtagClearsPreviousEtag() throws Exception {
390+
final FakeClient client =
391+
new FakeClient(
392+
response(200, "etag-a", emptyConfig()),
393+
response(200, null, emptyConfig()),
394+
response(304, null, null));
395+
final AgentlessConfigurationSource service = service(client);
396+
FeatureFlaggingGateway.addConfigListener(listener);
397+
398+
assertTrue(service.pollOnce());
399+
assertTrue(service.pollOnce());
400+
assertTrue(service.pollOnce());
401+
402+
assertEquals("etag-a", client.requests.get(1).etag);
403+
assertNull(client.requests.get(2).etag);
404+
verify(listener, times(2)).accept(any(ServerConfiguration.class));
405+
}
406+
388407
@Test
389408
void usesEtagAndSkipsDispatchOnUnchangedConfig() throws Exception {
390409
final FakeClient client =

0 commit comments

Comments
 (0)