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

Make disable_send dynamic, exclude health & config requests. #3352

Closed
wants to merge 1 commit into from
Closed

Make disable_send dynamic, exclude health & config requests. #3352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 1 addition & 1 deletion ...t-core/src/main/java/co/elastic/apm/agent/configuration/ApmServerConfigurationSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ String fetchConfig(final ConfigurationRegistry configurationRegistry) {
}
try {
payloadSerializer.blockUntilReady();
return apmServerClient.execute("/config/v1/agents", new ApmServerClient.ConnectionHandler<String>() {
return apmServerClient.forceExecute("/config/v1/agents", new ApmServerClient.ConnectionHandler<String>() {
@Override
public String withConnection(HttpURLConnection connection) throws IOException {
try {
Expand Down
11 changes: 9 additions & 2 deletions apm-agent-core/src/main/java/co/elastic/apm/agent/report/ApmServerClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,9 @@ private static List<URL> shuffleUrls(List<URL> serverUrls) {

@Nullable
HttpURLConnection startRequest(String relativePath) throws IOException {
if(reporterConfiguration.isSendDisabled()) {
return null;
}
URL url = appendPathToCurrentUrl(relativePath);
if (url == null) {
return null;
Expand Down Expand Up @@ -247,6 +250,7 @@ void onConnectionError() {
* If there's a connection error executing the request,
* the request is retried with the next APM Server url.
* The maximum amount of retries is the number of configured APM Server URLs.
* This method ignores the disable_send configuration.
*
* @param path the APM Server path
* @param connectionHandler receives the {@link HttpURLConnection} and returns the result
Expand All @@ -255,7 +259,7 @@ void onConnectionError() {
* @throws Exception in case all retries yield an exception, the last will be thrown
*/
@Nullable
public <V> V execute(String path, ConnectionHandler<V> connectionHandler) throws Exception {
public <V> V forceExecute(String path, ConnectionHandler<V> connectionHandler) throws Exception {
final List<URL> prioritizedUrlList = getPrioritizedUrlList();
if (prioritizedUrlList.isEmpty()) {
return null;
Expand Down Expand Up @@ -285,7 +289,10 @@ public <V> V execute(String path, ConnectionHandler<V> connectionHandler) throws
throw previousException;
}

public <T> List<T> executeForAllUrls(String path, ConnectionHandler<T> connectionHandler) {
/**
* Executes the given requests for all server URLs, even if the disable_send configuration is set to true.
*/
public <T> List<T> forceExecuteForAllUrls(String path, ConnectionHandler<T> connectionHandler) {
List<URL> serverUrls = getServerUrls();
List<T> results = new ArrayList<>(serverUrls.size());
for (URL serverUrl : serverUrls) {
Expand Down
2 changes: 1 addition & 1 deletion apm-agent-core/src/main/java/co/elastic/apm/agent/report/ApmServerHealthChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public Future<Version> checkHealthAndGetMinVersion() {
@Nullable
@Override
public Version call() {
List<Version> versions = apmServerClient.executeForAllUrls("/", new ApmServerClient.ConnectionHandler<Version>() {
List<Version> versions = apmServerClient.forceExecuteForAllUrls("/", new ApmServerClient.ConnectionHandler<Version>() {
@Override
public Version withConnection(HttpURLConnection connection) {
try {
Expand Down
7 changes: 4 additions & 3 deletions apm-agent-core/src/main/java/co/elastic/apm/agent/report/ReporterConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,9 +240,6 @@ public String getApiKey() {
* @return a list of APM Server URLs resulting from the combination of {@code server_url} and {@code server_urls}
*/
public List<URL> getServerUrls() {
if (disableSend.get()) {
return Collections.emptyList();
}
List<URL> calculatedUrlList;
URL singleUrl = serverUrl.get();
List<URL> urlList = serverUrls.get();
Expand All @@ -267,6 +264,10 @@ public List<URL> getServerUrls() {
return calculatedUrlList;
}

public boolean isSendDisabled() {
return disableSend.get();
}

public TimeDuration getServerTimeout() {
return serverTimeout.get();
}
Expand Down
47 changes: 25 additions & 22 deletions apm-agent-core/src/test/java/co/elastic/apm/agent/report/ApmServerClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ private static ConditionFactory awaitUpToOneSecond() {
public void testInitialCurrentUrlIsFirstUrl() throws Exception {
assertThat(Objects.requireNonNull(apmServerClient.getCurrentUrl()).getPort()).isEqualTo(apmServer1.port());

apmServerClient.execute("/test", HttpURLConnection::getResponseCode);
apmServerClient.forceExecute("/test", HttpURLConnection::getResponseCode);

apmServer1.verify(1, getRequestedFor(urlEqualTo("/test")));
apmServer2.verify(0, getRequestedFor(urlEqualTo("/test")));
Expand All @@ -215,7 +215,7 @@ public void testUseNextUrlOnError() throws Exception {
apmServerClient.incrementAndGetErrorCount(0);
assertThat(Objects.requireNonNull(apmServerClient.getCurrentUrl()).getPort()).isEqualTo(apmServer2.port());

apmServerClient.execute("/test", HttpURLConnection::getResponseCode);
apmServerClient.forceExecute("/test", HttpURLConnection::getResponseCode);

apmServer1.verify(0, getRequestedFor(urlEqualTo("/test")));
apmServer2.verify(1, getRequestedFor(urlEqualTo("/proxy/test")));
Expand All @@ -233,7 +233,7 @@ public void testWrapUrlsOnConsecutiveError() throws Exception {

@Test
public void testRetry() throws Exception {
assertThat(apmServerClient.<String>execute("/test", conn -> new String(conn.getInputStream().readAllBytes()))).isEqualTo("hello from server 2");
assertThat(apmServerClient.<String>forceExecute("/test", conn -> new String(conn.getInputStream().readAllBytes()))).isEqualTo("hello from server 2");
assertThat(Objects.requireNonNull(apmServerClient.getCurrentUrl()).getPort()).isEqualTo(apmServer2.port());
apmServer1.verify(1, getRequestedFor(urlEqualTo("/test")));
apmServer2.verify(1, getRequestedFor(urlEqualTo("/proxy/test")));
Expand All @@ -242,7 +242,7 @@ public void testRetry() throws Exception {

@Test
public void testRetryFailure() {
assertThatThrownBy(() -> apmServerClient.execute("/not-found", URLConnection::getInputStream))
assertThatThrownBy(() -> apmServerClient.forceExecute("/not-found", URLConnection::getInputStream))
.isInstanceOf(FileNotFoundException.class)
.matches(t -> t.getSuppressed().length == 1, "should have a suppressed exception");
apmServer1.verify(1, getRequestedFor(urlEqualTo("/not-found")));
Expand All @@ -254,7 +254,7 @@ public void testRetryFailure() {

@Test
public void testExecuteSuccessfullyForAllUrls() {
apmServerClient.executeForAllUrls("/not-found", connection -> {
apmServerClient.forceExecuteForAllUrls("/not-found", connection -> {
connection.getResponseCode();
return null;
});
Expand All @@ -268,7 +268,7 @@ public void testExecuteSuccessfullyForAllUrls() {
@Test
public void testExecuteFailureForAllUrls() {
// exception will only be logged, not thrown
apmServerClient.executeForAllUrls("/not-found", connection -> {
apmServerClient.forceExecuteForAllUrls("/not-found", connection -> {
connection.getInputStream();
return null;
});
Expand Down Expand Up @@ -320,17 +320,20 @@ public void testServerUrlSettingOverridesServerUrls() throws IOException {
}

@Test
public void testDisableSend() {
// We have to go through that because the disable_send config is non-dynamic
ConfigurationRegistry localConfig = SpyConfiguration.createSpyConfig(
Objects.requireNonNull(ConfigSources.fromClasspath("test.elasticapm.disable-send.properties", ClassLoader.getSystemClassLoader()))
);
final ElasticApmTracer tracer = new ElasticApmTracerBuilder()
.reporter(new MockReporter())
.configurationRegistry(localConfig)
.buildAndStart();
List<URL> updatedServerUrls = tracer.getApmServerClient().getServerUrls();
assertThat(updatedServerUrls).isEmpty();
public void testDisableSend() throws Exception {
doReturn(true).when(reporterConfiguration).isSendDisabled();

apmServerClient.forceExecute("/test", HttpURLConnection::getResponseCode);
assertThat(apmServerClient.startRequest("/test")).isNull();
apmServer1.verify(1, getRequestedFor(urlEqualTo("/test")));

doReturn(false).when(reporterConfiguration).isSendDisabled();

apmServerClient.forceExecute("/test", HttpURLConnection::getResponseCode);
HttpURLConnection request = apmServerClient.startRequest("/test");
assertThat(request).isNotNull();
request.getResponseCode();
apmServer1.verify(3, getRequestedFor(urlEqualTo("/test")));
}

@Test
Expand All @@ -351,7 +354,7 @@ public void testWithEmptyServerUrlList() {
client.start(Collections.emptyList());
Exception exception = null;
try {
client.execute("/irrelevant", connection -> null);
client.forceExecute("/irrelevant", connection -> null);
} catch (Exception e) {
exception = e;
}
Expand All @@ -376,13 +379,13 @@ public void testSupportUnsampledTransactions() {
public void testApiKeyRotation() throws Exception {
doReturn("token1").when(reporterConfiguration).getApiKey();

apmServerClient.execute("/test", HttpURLConnection::getResponseCode);
apmServerClient.forceExecute("/test", HttpURLConnection::getResponseCode);
apmServer1.verify(1, getRequestedFor(urlEqualTo("/test"))
.withHeader("Authorization", equalTo("ApiKey token1")));

doReturn("token2").when(reporterConfiguration).getApiKey();

apmServerClient.execute("/test", HttpURLConnection::getResponseCode);
apmServerClient.forceExecute("/test", HttpURLConnection::getResponseCode);
apmServer1.verify(1, getRequestedFor(urlEqualTo("/test"))
.withHeader("Authorization", equalTo("ApiKey token2")));
}
Expand All @@ -392,13 +395,13 @@ public void testApiKeyRotation() throws Exception {
public void testSecretTokenRotation() throws Exception {
doReturn("token1").when(reporterConfiguration).getSecretToken();

apmServerClient.execute("/test", HttpURLConnection::getResponseCode);
apmServerClient.forceExecute("/test", HttpURLConnection::getResponseCode);
apmServer1.verify(1, getRequestedFor(urlEqualTo("/test"))
.withHeader("Authorization", equalTo("Bearer token1")));

doReturn("token2").when(reporterConfiguration).getSecretToken();

apmServerClient.execute("/test", HttpURLConnection::getResponseCode);
apmServerClient.forceExecute("/test", HttpURLConnection::getResponseCode);
apmServer1.verify(1, getRequestedFor(urlEqualTo("/test"))
.withHeader("Authorization", equalTo("Bearer token2")));
}
Expand Down
2 changes: 1 addition & 1 deletion apm-agent-core/src/test/java/specs/MockServerStepDefinitions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public void executeRequest() throws IOException {
ApmServerClient apmServerClient = new ApmServerClient(config);
apmServerClient.start();
try {
apmServerClient.execute("/", new ApmServerClient.ConnectionHandler<>() {
apmServerClient.forceExecute("/", new ApmServerClient.ConnectionHandler<>() {
@Nullable
@Override
public Object withConnection(HttpURLConnection connection) throws IOException {
Expand Down
Loading