make server_urls dynamically reloadable #734

videnkz · 2019-07-19T09:24:49Z

closes #723

Checklist

felixbarny

Unfortunately, it's not as easy as that. The ApmServerClient#serverUrls variable has to be updated as well, in a thread-safe manner. Meaning that the variable should be declared volatile.

See also #725 (comment) for more details on how to use change listeners on how to register a config change callback. You can do that in the constructor of ApmServerClient. Also, reset the error count when the config changes.

…latile.

videnkz · 2019-07-19T10:32:25Z

Unfortunately, it's not as easy as that. The ApmServerClient#serverUrls variable has to be updated as well, in a thread-safe manner. Meaning that the variable should be declared volatile.

See also #725 (comment) for more details on how to use change listeners on how to register a config change callback. You can do that in the constructor of ApmServerClient. Also, reset the error count when the config changes.

Also, reset the err

@felixbarny
I added listener in constructor.

videnkz · 2019-07-19T10:36:57Z

success log example:

2019-07-19 16:51:59.323 [apm-reporter] DEBUG co.elastic.apm.agent.report.IntakeV2ReportingEventHandler - Receiving TRANSACTION event (sequence 3)
2019-07-19 16:51:59.324 [apm-reporter] DEBUG co.elastic.apm.agent.report.IntakeV2ReportingEventHandler - Starting new request to http://localhost:8201/intake/v2/events
2019-07-19 16:51:59.324 [apm-reporter] ERROR co.elastic.apm.agent.report.IntakeV2ReportingEventHandler - Failed to handle event of type TRANSACTION with this error: Connection refused (Connection refused)
2019-07-19 16:51:59.324 [apm-reporter] DEBUG co.elastic.apm.agent.report.IntakeV2ReportingEventHandler - Event handling failure
java.net.ConnectException: Connection refused (Connection refused)
        at java.net.PlainSocketImpl.socketConnect(Native Method)
        at java.net.AbstractPlainSocketImpl.doConnect(AbstractPlainSocketImpl.java:350)
        at java.net.AbstractPlainSocketImpl.connectToAddress(AbstractPlainSocketImpl.java:206)
        at java.net.AbstractPlainSocketImpl.connect(AbstractPlainSocketImpl.java:188)
        at java.net.SocksSocketImpl.connect(SocksSocketImpl.java:392)
        at java.net.Socket.connect(Socket.java:589)
        at sun.net.NetworkClient.doConnect(NetworkClient.java:175)
        at sun.net.www.http.HttpClient.openServer(HttpClient.java:463)
        at sun.net.www.http.HttpClient.openServer(HttpClient.java:558)
        at sun.net.www.http.HttpClient.<init>(HttpClient.java:242)
        at sun.net.www.http.HttpClient.New(HttpClient.java:339)
        at sun.net.www.http.HttpClient.New(HttpClient.java:357)
        at sun.net.www.protocol.http.HttpURLConnection.getNewHttpClient(HttpURLConnection.java:1220)
        at sun.net.www.protocol.http.HttpURLConnection.plainConnect0(HttpURLConnection.java:1156)
        at sun.net.www.protocol.http.HttpURLConnection.plainConnect(HttpURLConnection.java:1050)
        at sun.net.www.protocol.http.HttpURLConnection.connect(HttpURLConnection.java:984)
        at co.elastic.apm.agent.report.IntakeV2ReportingEventHandler.startRequest(IntakeV2ReportingEventHandler.java:200)
        at co.elastic.apm.agent.report.IntakeV2ReportingEventHandler.handleEvent(IntakeV2ReportingEventHandler.java:138)
        at co.elastic.apm.agent.report.IntakeV2ReportingEventHandler.onEvent(IntakeV2ReportingEventHandler.java:116)
        at co.elastic.apm.agent.report.IntakeV2ReportingEventHandler.onEvent(IntakeV2ReportingEventHandler.java:50)
        at co.elastic.apm.agent.shaded.lmax.disruptor.BatchEventProcessor.processEvents(BatchEventProcessor.java:168)
        at co.elastic.apm.agent.shaded.lmax.disruptor.BatchEventProcessor.run(BatchEventProcessor.java:125)
        at java.lang.Thread.run(Thread.java:748)
2019-07-19 16:51:59.324 [apm-reporter] INFO co.elastic.apm.agent.report.IntakeV2ReportingEventHandler - Backing off for 9 seconds (+/-10%)
2019-07-19 16:52:00.223 [http-nio-8085-exec-6] DEBUG co.elastic.apm.agent.impl.transaction.AbstractSpan - increment references to '' 00-1f232781dea1331ae9d44d0ea992294b-db24862afff8a75f-01 (343dbcb6) (1)
2019-07-19 16:52:00.223 [http-nio-8085-exec-6] DEBUG co.elastic.apm.agent.impl.ElasticApmTracer - startTransaction '' 00-1f232781dea1331ae9d44d0ea992294b-db24862afff8a75f-01 (343dbcb6) {
2019-07-19 16:52:00.223 [http-nio-8085-exec-6] DEBUG co.elastic.apm.agent.impl.transaction.AbstractSpan - increment references to '' 00-1f232781dea1331ae9d44d0ea992294b-db24862afff8a75f-01 (343dbcb6) (2)
2019-07-19 16:52:00.223 [http-nio-8085-exec-6] DEBUG co.elastic.apm.agent.impl.ElasticApmTracer - Activating '' 00-1f232781dea1331ae9d44d0ea992294b-db24862afff8a75f-01 (343dbcb6) on thread 33
2019-07-19 16:52:00.224 [http-nio-8085-exec-6] DEBUG co.elastic.apm.agent.spring.webmvc.SpringTransactionNameInstrumentation - Set name ServiceAController#pingPong to transaction db24862afff8a75f
2019-07-19 16:52:00.225 [http-nio-8085-exec-6] DEBUG co.elastic.apm.agent.impl.ElasticApmTracer - Deactivating 'ServiceAController#pingPong' 00-1f232781dea1331ae9d44d0ea992294b-db24862afff8a75f-01 (343dbcb6) on thread 33
2019-07-19 16:52:00.225 [http-nio-8085-exec-6] DEBUG co.elastic.apm.agent.impl.transaction.AbstractSpan - decrement references to 'ServiceAController#pingPong' 00-1f232781dea1331ae9d44d0ea992294b-db24862afff8a75f-01 (343dbcb6) (1)
2019-07-19 16:52:00.226 [http-nio-8085-exec-6] DEBUG co.elastic.apm.agent.impl.ElasticApmTracer - } endTransaction 'ServiceAController#pingPong' 00-1f232781dea1331ae9d44d0ea992294b-db24862afff8a75f-01 (343dbcb6)
2019-07-19 16:52:00.780 [configuration-reloader] DEBUG co.elastic.apm.agent.report.ApmServerClient - server_urls override with value = ([http://localhost:8200]).
2019-07-19 16:52:08.505 [apm-reporter] DEBUG co.elastic.apm.agent.report.IntakeV2ReportingEventHandler - Receiving TRANSACTION event (sequence 4)
2019-07-19 16:52:08.505 [apm-reporter] DEBUG co.elastic.apm.agent.report.IntakeV2ReportingEventHandler - Starting new request to http://localhost:8200/intake/v2/events
2019-07-19 16:52:08.516 [apm-reporter] DEBUG co.elastic.apm.agent.report.IntakeV2ReportingEventHandler - Scheduling request timeout in 10s
2019-07-19 16:52:08.523 [apm-reporter] DEBUG co.elastic.apm.agent.impl.transaction.AbstractSpan - decrement references to 'ServiceAController#pingPong' 00-1f232781dea1331ae9d44d0ea992294b-db24862afff8a75f-01 (343dbcb6) (0)
2019-07-19 16:52:08.523 [apm-reporter] DEBUG co.elastic.apm.agent.report.IntakeV2ReportingEventHandler - Receiving METRICS event (sequence 5)
2019-07-19 16:52:18.517 [apm-request-timeout-timer] DEBUG co.elastic.apm.agent.report.IntakeV2ReportingEventHandler - Request flush because the request timeout occurred
2019-07-19 16:52:18.520 [apm-reporter] DEBUG co.elastic.apm.agent.report.IntakeV2ReportingEventHandler - Receiving FLUSH event (sequence 6)
2019-07-19 16:52:18.521 [apm-reporter] DEBUG co.elastic.apm.agent.report.IntakeV2ReportingEventHandler - Flushing 2313 uncompressed 1112 compressed bytes
2019-07-19 16:52:30.854 [apm-reporter] DEBUG co.elastic.apm.agent.report.IntakeV2ReportingEventHandler - Receiving METRICS event (sequence 7)

felixbarny · 2019-07-19T11:26:55Z

apm-agent-core/src/main/java/co/elastic/apm/agent/report/ApmServerClient.java

@@ -73,6 +74,16 @@ public ApmServerClient(ReporterConfiguration reporterConfiguration) {

    public ApmServerClient(ReporterConfiguration reporterConfiguration, List<URL> serverUrls) {
        this.reporterConfiguration = reporterConfiguration;
+        for (ConfigurationOption configurationOption : reporterConfiguration.getConfigurationOptions()) {


Better create a method public ConfigurationOption<List<URL>> getServerUrlsOption() in ReporterConfiguration and then just call reporterConfiguration.getServerUrlsOption().addChangeListener(...) here

felixbarny · 2019-07-19T11:28:25Z

apm-agent-core/src/main/java/co/elastic/apm/agent/report/ApmServerClient.java

@@ -245,4 +256,10 @@ int getErrorCount() {
        T withConnection(HttpURLConnection connection) throws IOException;
    }

+    public synchronized void overrideApmServerUrls(List<URL> serverUrls) {


Just inline this to the onChange method. Doesn't have to be synchronized.

felixbarny · 2019-07-19T11:31:02Z

apm-agent-core/src/main/java/co/elastic/apm/agent/report/ApmServerClient.java

@@ -245,4 +256,10 @@ int getErrorCount() {
        T withConnection(HttpURLConnection connection) throws IOException;
    }

+    public synchronized void overrideApmServerUrls(List<URL> serverUrls) {
+        logger.debug("server_urls override with value = ({}).", serverUrls);
+        this.serverUrls = serverUrls;


Shuffle the URLs by reusing the shuffleUrls method. To achieve that, change the parameter of shuffleUrls from ReporterConfiguration to List<URL>.

videnkz · 2019-07-19T11:58:55Z

@felixbarny I fixed according to comments

felixbarny

Thanks ❤️

felixbarny · 2019-07-19T12:02:54Z

could you add a section to the changelog?

videnkz · 2019-07-19T12:14:46Z

could you add a section to the changelog?

added

eyalkoren

Thanks @kananindzya!!

We strive to adding a unit test to any new functionality, where applicable. You can add a package-private method ApmServerClient#getServerUrls to make the server url list available and add a unit test to ApmServerClientTest.
A good way to unit-test it is by changing it through system properties, then reload and see that the list had changed. For that, you will have to initialize an ElasticApmTracer and use the ReporterConfiguration from it when creating the test's client (as opposed to the current way the client is created with a new ReporterConfiguration, and after changing the setting reload through ElasticApmTracer#getConfigurationRegistry().reload("server_urls").
Just don't forget to restore at the end of the test, so that the other tests will not be affected.

felixbarny · 2019-07-22T07:14:41Z

Easier than using an actual properties file for testing is to use an in-memory PropertySource like here:

apm-agent-java/apm-agent-plugins/apm-slf4j-plugin/src/test/java/co/elastic/apm/agent/slf4j/Slf4JMdcActivationListenerTest.java

Line 175 in 45857ce

    
           config.save("enable_log_correlation", "true", SpyConfiguration.CONFIG_SOURCE_NAME);

…indzya/apm-agent-java into server_urls_dynamically_reloadable

videnkz · 2019-07-22T11:37:17Z

getServerUrls

hi @eyalkoren , I added protected ApmServerClient#getServerUrls, and added test with using

config.save("server_urls", tempUrl.toString(), SpyConfiguration.CONFIG_SOURCE_NAME);

eyalkoren

Excellent 👌 !
Few minor last comments about accessibility and unused imports.
I prefer the most restricted access, especially for something we expose for testing purposes. Package private is more "accurate" than protected in this case.

eyalkoren · 2019-07-22T11:30:51Z

apm-agent-core/src/test/java/co/elastic/apm/agent/report/ApmServerClientTest.java

 import java.util.List;
+import java.util.ServiceLoader;


remove unused imports

apm-agent-core/src/test/java/co/elastic/apm/agent/report/ApmServerClientTest.java

apm-agent-core/src/main/java/co/elastic/apm/agent/report/ApmServerClient.java

…rverClientTest.java Co-Authored-By: eyalkoren <41850454+eyalkoren@users.noreply.github.com>

…rverClient.java Co-Authored-By: eyalkoren <41850454+eyalkoren@users.noreply.github.com>

eyalkoren

Thanks again @kananindzya !

felixbarny · 2019-07-22T12:23:33Z

can you merge in the changes from master again? that should fix the test failures

felixbarny · 2019-07-22T12:24:48Z

run the tests

make server_urls dynamically reloadable

dc99483

felixbarny requested changes Jul 19, 2019

View reviewed changes

added listener on properties change. Make serverUrls as static and vo…

12845ec

…latile.

moved to consturoctor. added reseting of errorCount

ab64e66

felixbarny reviewed Jul 19, 2019

View reviewed changes

fixed according to comments

b71827b

felixbarny approved these changes Jul 19, 2019

View reviewed changes

added feature to changelog

2811873

eyalkoren reviewed Jul 21, 2019

View reviewed changes

felixbarny added the [zube]: In Review label Jul 22, 2019

kananindzya and others added 4 commits July 22, 2019 17:01

added test for checking dynamic change of server_urls

35491ea

Merge branch 'master' into server_urls_dynamically_reloadable

e11deae

delete sout

75fc77f

Merge branch 'server_urls_dynamically_reloadable' of github.com:kanan…

e5c527b

…indzya/apm-agent-java into server_urls_dynamically_reloadable

eyalkoren reviewed Jul 22, 2019

View reviewed changes

videnkz and others added 3 commits July 22, 2019 17:50

Update apm-agent-core/src/test/java/co/elastic/apm/agent/report/ApmSe…

f2df153

…rverClientTest.java Co-Authored-By: eyalkoren <41850454+eyalkoren@users.noreply.github.com>

Update apm-agent-core/src/main/java/co/elastic/apm/agent/report/ApmSe…

0856c56

…rverClient.java Co-Authored-By: eyalkoren <41850454+eyalkoren@users.noreply.github.com>

fix according to comments

a2313f5

eyalkoren approved these changes Jul 22, 2019

View reviewed changes

eyalkoren merged commit 5fd874d into elastic:master Jul 22, 2019

zube bot removed the [zube]: In Review label Jul 22, 2019

zube bot added the [zube]: Done label Jul 22, 2019

lreuven removed the [zube]: Done label Nov 18, 2019

make server_urls dynamically reloadable #734

make server_urls dynamically reloadable #734

Uh oh!

Conversation

videnkz commented Jul 19, 2019 • edited by eyalkoren Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

felixbarny left a comment

Choose a reason for hiding this comment

Uh oh!

videnkz commented Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

videnkz commented Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

videnkz commented Jul 19, 2019

Uh oh!

felixbarny left a comment

Choose a reason for hiding this comment

Uh oh!

felixbarny commented Jul 19, 2019

Uh oh!

videnkz commented Jul 19, 2019

Uh oh!

eyalkoren left a comment

Choose a reason for hiding this comment

Uh oh!

felixbarny commented Jul 22, 2019

Uh oh!

videnkz commented Jul 22, 2019

Uh oh!

eyalkoren 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!

Uh oh!

Uh oh!

eyalkoren left a comment

Choose a reason for hiding this comment

Uh oh!

felixbarny commented Jul 22, 2019

Uh oh!

felixbarny commented Jul 22, 2019

Uh oh!

Uh oh!

videnkz commented Jul 19, 2019 •

edited by eyalkoren

Loading

videnkz commented Jul 19, 2019 •

edited

Loading

videnkz commented Jul 19, 2019 •

edited

Loading