From fc9c2649cff915e574c815a86acff14345836b9e Mon Sep 17 00:00:00 2001 From: Jack Shirazi Date: Sun, 26 Feb 2023 19:32:35 +0000 Subject: [PATCH 1/4] add agent config access to the plugin sdk --- .../java/co/elastic/apm/api/ElasticApm.java | 38 +++++++ .../ElasticApmApiInstrumentation.java | 22 ++++ ...ic.apm.agent.sdk.ElasticApmInstrumentation | 1 + .../pluginapi/ConfigInstrumentationTest.java | 106 ++++++++++++++++++ 4 files changed, 167 insertions(+) create mode 100644 apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/agent/pluginapi/ConfigInstrumentationTest.java diff --git a/apm-agent-api/src/main/java/co/elastic/apm/api/ElasticApm.java b/apm-agent-api/src/main/java/co/elastic/apm/api/ElasticApm.java index 0610be92b4..f846f70ef1 100644 --- a/apm-agent-api/src/main/java/co/elastic/apm/api/ElasticApm.java +++ b/apm-agent-api/src/main/java/co/elastic/apm/api/ElasticApm.java @@ -249,4 +249,42 @@ public static void captureException(@Nullable Throwable e) { public static void setServiceInfoForClassLoader(@Nullable ClassLoader classLoader, @Nullable String serviceName, @Nullable String serviceVersion) { // co.elastic.apm.api.ElasticApmInstrumentation.SetServiceInfoForClassLoader.setServiceInfoForClassLoader } + + /** + * Returns the current value of a config option (regardless of however and whenever that option was (re)set) + *

+ * This applies to dynamically updated values too, so for example "transaction_sample_rate" + * can be dynamically reset (eg by executing + * System.setProperty("elastic.apm.transaction_sample_rate", "0.6") + * or changing the "transaction_sample_rate" in the property file or from fleet UI) + * and calling getConfig("transaction_sample_rate") would provide the current value + *

+ *

+ * Note the value returned can be null if an invalid key is used, and also + * if the "public-api" set of instrumentations has not been applied + *

+ * + * @param key the string option + * @return The current value for the option, per the option type (String, Long, Integer, etc) + */ + public static Object getConfig(String key) { + // co.elastic.apm.api.ElasticApmInstrumentation.ConfigInstrumentation.getConfig + return null; + } + + /** + * Convenience method that just calls System.setProperty("elastic.apm."+key, value); + *

+ * Note there is a delay between executing the update and it taking effect. The corresponding + * {@link #getConfig(String)} ()} will only report the new value after it has taken effect. + * Note also that only dynamic options can be updated, other updates will simply be ignored. + *

+ * + * @param key the string option + * @param value the value for that option. No validation is applied here, so invalid + * values for an option will be ignored by the configuration system + */ + public static void setConfig(String key, String value) { + System.setProperty("elastic.apm."+key, value); + } } diff --git a/apm-agent-plugins/apm-api-plugin/src/main/java/co/elastic/apm/agent/pluginapi/ElasticApmApiInstrumentation.java b/apm-agent-plugins/apm-api-plugin/src/main/java/co/elastic/apm/agent/pluginapi/ElasticApmApiInstrumentation.java index c7cb07f4be..6e27850371 100644 --- a/apm-agent-plugins/apm-api-plugin/src/main/java/co/elastic/apm/agent/pluginapi/ElasticApmApiInstrumentation.java +++ b/apm-agent-plugins/apm-api-plugin/src/main/java/co/elastic/apm/agent/pluginapi/ElasticApmApiInstrumentation.java @@ -19,6 +19,7 @@ package co.elastic.apm.agent.pluginapi; import co.elastic.apm.agent.configuration.ServiceInfo; +import co.elastic.apm.agent.impl.GlobalTracer; import co.elastic.apm.agent.impl.transaction.Transaction; import co.elastic.apm.agent.util.PrivilegedActionUtils; import net.bytebuddy.asm.Advice; @@ -162,4 +163,25 @@ public static void setServiceInfoForClassLoader(@Advice.Argument(0) @Nullable Cl } } } + + public static class ConfigInstrumentation extends ElasticApmApiInstrumentation { + public ConfigInstrumentation() { + super(named("getConfig")); + } + + public static class AdviceClass { + @Nullable + @Advice.AssignReturned.ToReturned + @Advice.OnMethodExit(suppress = Throwable.class, inline = false) + public static Object getConfig(@Advice.Argument(0) @Nullable String key) { + try { + Object value= GlobalTracer.getTracerImpl().getConfigurationRegistry().getConfigurationOptionByKey(key).getValue(); + // should we return a special value for null - which means non-existent entry? + return value; + } catch (NullPointerException e) { + return null; + } + } + } + } } diff --git a/apm-agent-plugins/apm-api-plugin/src/main/resources/META-INF/services/co.elastic.apm.agent.sdk.ElasticApmInstrumentation b/apm-agent-plugins/apm-api-plugin/src/main/resources/META-INF/services/co.elastic.apm.agent.sdk.ElasticApmInstrumentation index 4497cb5f29..f117dfa6a8 100644 --- a/apm-agent-plugins/apm-api-plugin/src/main/resources/META-INF/services/co.elastic.apm.agent.sdk.ElasticApmInstrumentation +++ b/apm-agent-plugins/apm-api-plugin/src/main/resources/META-INF/services/co.elastic.apm.agent.sdk.ElasticApmInstrumentation @@ -4,6 +4,7 @@ co.elastic.apm.agent.pluginapi.ElasticApmApiInstrumentation$CurrentTransactionIn co.elastic.apm.agent.pluginapi.ElasticApmApiInstrumentation$CurrentSpanInstrumentation co.elastic.apm.agent.pluginapi.ElasticApmApiInstrumentation$CaptureExceptionInstrumentation co.elastic.apm.agent.pluginapi.ElasticApmApiInstrumentation$SetServiceInfoForClassLoaderInstrumentation +co.elastic.apm.agent.pluginapi.ElasticApmApiInstrumentation$ConfigInstrumentation co.elastic.apm.agent.pluginapi.TransactionInstrumentation$SetFrameworkNameInstrumentation co.elastic.apm.agent.pluginapi.TransactionInstrumentation$SetUserInstrumentation co.elastic.apm.agent.pluginapi.TransactionInstrumentation$EnsureParentIdInstrumentation diff --git a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/agent/pluginapi/ConfigInstrumentationTest.java b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/agent/pluginapi/ConfigInstrumentationTest.java new file mode 100644 index 0000000000..0141207d11 --- /dev/null +++ b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/agent/pluginapi/ConfigInstrumentationTest.java @@ -0,0 +1,106 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package co.elastic.apm.agent.pluginapi; + +import co.elastic.apm.agent.MockTracer; +import co.elastic.apm.agent.bci.ElasticApmAgent; +import co.elastic.apm.agent.bci.TracerAwareInstrumentation; +import co.elastic.apm.agent.configuration.CoreConfiguration; +import co.elastic.apm.agent.impl.ElasticApmTracer; +import co.elastic.apm.agent.sdk.ElasticApmInstrumentation; +import co.elastic.apm.api.ElasticApm; +import net.bytebuddy.agent.ByteBuddyAgent; +import net.bytebuddy.asm.Advice; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; +import net.bytebuddy.matcher.ElementMatchers; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.stagemonitor.configuration.ConfigurationRegistry; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; + +import static net.bytebuddy.matcher.ElementMatchers.named; +import static org.assertj.core.api.Assertions.assertThat; + +class ConfigInstrumentationTest { + + private static final Integer INVALID_TRANSACTION_MAX_SPANS = -37; + + private ElasticApmTracer tracer; + private ConfigurationRegistry configurationRegistry; + private CoreConfiguration coreConfig; + + @BeforeEach + void setup() { + tracer = MockTracer.createRealTracer(); + configurationRegistry = tracer.getConfigurationRegistry(); + coreConfig = configurationRegistry.getConfig(CoreConfiguration.class); + } + + @AfterEach + void reset() { + ElasticApmAgent.reset(); + } + + @Test + void testConfigAccess() { + ArrayList instrumentations = new ArrayList<>(); + instrumentations.add(new MaxTransInstrumentation()); + instrumentations.add(new ElasticApmApiInstrumentation.ConfigInstrumentation()); + ElasticApmAgent.initInstrumentation(tracer, ByteBuddyAgent.install(), instrumentations); + assertThat(coreConfig.getTransactionMaxSpans()).isNotEqualTo(INVALID_TRANSACTION_MAX_SPANS); + assertThat(configValue()).isEqualTo(coreConfig.getTransactionMaxSpans()); + } + + Integer configValue() { + return INVALID_TRANSACTION_MAX_SPANS; + } + + public static class MaxTransInstrumentation extends TracerAwareInstrumentation { + public static class AdviceClass { + @Advice.AssignReturned.ToReturned + @Advice.OnMethodExit(inline = false) + public static Integer onMethodExit() { + return (Integer) ElasticApm.getConfig("transaction_max_spans"); + } + } + + @Override + public ElementMatcher getTypeMatcher() { + return named(ConfigInstrumentationTest.class.getName()); + } + + @Override + public ElementMatcher getMethodMatcher() { + return ElementMatchers.nameEndsWithIgnoreCase("configValue"); + } + + @Override + public Collection getInstrumentationGroupNames() { + return Collections.emptyList(); + } + + } + +} From cc28b3eb98ee8e3d2b8e16b46291c975916c97f2 Mon Sep 17 00:00:00 2001 From: Jack Shirazi Date: Mon, 27 Feb 2023 09:14:35 +0000 Subject: [PATCH 2/4] specify 'since' version --- apm-agent-api/src/main/java/co/elastic/apm/api/ElasticApm.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/apm-agent-api/src/main/java/co/elastic/apm/api/ElasticApm.java b/apm-agent-api/src/main/java/co/elastic/apm/api/ElasticApm.java index f846f70ef1..43086f9da3 100644 --- a/apm-agent-api/src/main/java/co/elastic/apm/api/ElasticApm.java +++ b/apm-agent-api/src/main/java/co/elastic/apm/api/ElasticApm.java @@ -266,6 +266,7 @@ public static void setServiceInfoForClassLoader(@Nullable ClassLoader classLoade * * @param key the string option * @return The current value for the option, per the option type (String, Long, Integer, etc) + * @since 1.37.0 */ public static Object getConfig(String key) { // co.elastic.apm.api.ElasticApmInstrumentation.ConfigInstrumentation.getConfig @@ -283,6 +284,7 @@ public static Object getConfig(String key) { * @param key the string option * @param value the value for that option. No validation is applied here, so invalid * values for an option will be ignored by the configuration system + * @since 1.37.0 */ public static void setConfig(String key, String value) { System.setProperty("elastic.apm."+key, value); From 0ec24f425623cfcff1d0f2cb2b7e125fe24df2b5 Mon Sep 17 00:00:00 2001 From: Jack Shirazi Date: Wed, 1 Mar 2023 19:14:06 +0000 Subject: [PATCH 3/4] better signature, throw exception for invalid calls, add tests --- .../java/co/elastic/apm/api/ElasticApm.java | 28 ++++++++++--- .../ElasticApmApiInstrumentation.java | 17 +++++--- .../pluginapi/ConfigInstrumentationTest.java | 39 ++++++++++++++++--- 3 files changed, 68 insertions(+), 16 deletions(-) diff --git a/apm-agent-api/src/main/java/co/elastic/apm/api/ElasticApm.java b/apm-agent-api/src/main/java/co/elastic/apm/api/ElasticApm.java index 43086f9da3..2545f73949 100644 --- a/apm-agent-api/src/main/java/co/elastic/apm/api/ElasticApm.java +++ b/apm-agent-api/src/main/java/co/elastic/apm/api/ElasticApm.java @@ -260,16 +260,34 @@ public static void setServiceInfoForClassLoader(@Nullable ClassLoader classLoade * and calling getConfig("transaction_sample_rate") would provide the current value *

*

- * Note the value returned can be null if an invalid key is used, and also - * if the "public-api" set of instrumentations has not been applied + * If an invalid key is passed as the first argument, or if the incorrect type for + * the key is passed as the second argument, the method will throw an IllegalArgumentException + *

+ *

+ * Note the value returned can be null if the "public-api" set of instrumentations + * has not been applied and also if the agent hasn't finished initializing *

* * @param key the string option + * @param type the type of the option (String, Long, Integer, etc) * @return The current value for the option, per the option type (String, Long, Integer, etc) * @since 1.37.0 */ - public static Object getConfig(String key) { - // co.elastic.apm.api.ElasticApmInstrumentation.ConfigInstrumentation.getConfig + public static T getConfig(String key, Class type) { + Object value = doGetConfig(key); + if (value == null) { + throw new IllegalArgumentException("There is no such option: " + key); + } else if (value instanceof IllegalStateException) { + return null; + } else if (value.getClass().equals(type)) { + return (T) value; + } else { + throw new IllegalArgumentException("The option: '"+key+"' is not of type "+type.getName()); + } + } + + private static Object doGetConfig(String key) { + // co.elastic.apm.api.ElasticApmInstrumentation.ConfigInstrumentation.doGetConfig return null; } @@ -277,7 +295,7 @@ public static Object getConfig(String key) { * Convenience method that just calls System.setProperty("elastic.apm."+key, value); *

* Note there is a delay between executing the update and it taking effect. The corresponding - * {@link #getConfig(String)} ()} will only report the new value after it has taken effect. + * {@link #getConfig(String, Class)} ()} will only report the new value after it has taken effect. * Note also that only dynamic options can be updated, other updates will simply be ignored. *

* diff --git a/apm-agent-plugins/apm-api-plugin/src/main/java/co/elastic/apm/agent/pluginapi/ElasticApmApiInstrumentation.java b/apm-agent-plugins/apm-api-plugin/src/main/java/co/elastic/apm/agent/pluginapi/ElasticApmApiInstrumentation.java index 6e27850371..841adfbed4 100644 --- a/apm-agent-plugins/apm-api-plugin/src/main/java/co/elastic/apm/agent/pluginapi/ElasticApmApiInstrumentation.java +++ b/apm-agent-plugins/apm-api-plugin/src/main/java/co/elastic/apm/agent/pluginapi/ElasticApmApiInstrumentation.java @@ -26,6 +26,7 @@ import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; +import org.stagemonitor.configuration.ConfigurationOption; import javax.annotation.Nullable; import java.lang.invoke.MethodHandle; @@ -166,20 +167,24 @@ public static void setServiceInfoForClassLoader(@Advice.Argument(0) @Nullable Cl public static class ConfigInstrumentation extends ElasticApmApiInstrumentation { public ConfigInstrumentation() { - super(named("getConfig")); + super(named("doGetConfig")); } public static class AdviceClass { @Nullable @Advice.AssignReturned.ToReturned @Advice.OnMethodExit(suppress = Throwable.class, inline = false) - public static Object getConfig(@Advice.Argument(0) @Nullable String key) { + public static Object doGetConfig(@Advice.Argument(0) @Nullable String key) { try { - Object value= GlobalTracer.getTracerImpl().getConfigurationRegistry().getConfigurationOptionByKey(key).getValue(); - // should we return a special value for null - which means non-existent entry? - return value; + ConfigurationOption configValue = GlobalTracer.getTracerImpl().getConfigurationRegistry().getConfigurationOptionByKey(key); + if (configValue == null) { + return null; + } else { + return configValue.getValue(); + } } catch (NullPointerException e) { - return null; + //this can only happen if the agent is messed up or not yet initialized + return new IllegalStateException("The agent is not initialized"); } } } diff --git a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/agent/pluginapi/ConfigInstrumentationTest.java b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/agent/pluginapi/ConfigInstrumentationTest.java index 0141207d11..12690a19b3 100644 --- a/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/agent/pluginapi/ConfigInstrumentationTest.java +++ b/apm-agent-plugins/apm-api-plugin/src/test/java/co/elastic/apm/agent/pluginapi/ConfigInstrumentationTest.java @@ -36,12 +36,15 @@ import org.junit.jupiter.api.Test; import org.stagemonitor.configuration.ConfigurationRegistry; +import javax.annotation.Nullable; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import static net.bytebuddy.matcher.ElementMatchers.named; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatException; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; class ConfigInstrumentationTest { @@ -64,16 +67,36 @@ void reset() { } @Test - void testConfigAccess() { + void testValidOptionConfigAccess() { + initAgent(); + assertThat(configValue(Boolean.TRUE)).isEqualTo(coreConfig.getTransactionMaxSpans()); + } + + @Test + void testInvalidOptionConfigAccess() { + initAgent(); + assertThatExceptionOfType(IllegalArgumentException.class) + .isThrownBy(() -> configValue(Boolean.FALSE)) + .withMessageStartingWith("There is no such option"); + } + + @Test + void testInvalidTypeConfigAccess() { + initAgent(); + assertThatExceptionOfType(IllegalArgumentException.class) + .isThrownBy(() -> configValue(null)) + .withMessageContaining("is not of type"); + } + + private void initAgent() { ArrayList instrumentations = new ArrayList<>(); instrumentations.add(new MaxTransInstrumentation()); instrumentations.add(new ElasticApmApiInstrumentation.ConfigInstrumentation()); ElasticApmAgent.initInstrumentation(tracer, ByteBuddyAgent.install(), instrumentations); assertThat(coreConfig.getTransactionMaxSpans()).isNotEqualTo(INVALID_TRANSACTION_MAX_SPANS); - assertThat(configValue()).isEqualTo(coreConfig.getTransactionMaxSpans()); } - Integer configValue() { + Integer configValue(Boolean valid) { return INVALID_TRANSACTION_MAX_SPANS; } @@ -81,8 +104,14 @@ public static class MaxTransInstrumentation extends TracerAwareInstrumentation { public static class AdviceClass { @Advice.AssignReturned.ToReturned @Advice.OnMethodExit(inline = false) - public static Integer onMethodExit() { - return (Integer) ElasticApm.getConfig("transaction_max_spans"); + public static Integer onMethodExit(@Advice.Argument(0) @Nullable Boolean valid) { + if (valid == null) { + return ElasticApm.getConfig("transaction_max_spans", Long.class).intValue(); + } else if (valid.booleanValue()) { + return ElasticApm.getConfig("transaction_max_spans", Integer.class); + } else { //if (valid.booleanValue()) - no other options + return ElasticApm.getConfig("xyz", Integer.class); + } } } From 016450f2e7bda6e0cc2e8cc4ced0d5c66e586165 Mon Sep 17 00:00:00 2001 From: Jack Shirazi Date: Thu, 2 Mar 2023 13:07:27 +0000 Subject: [PATCH 4/4] inherited match instead of exact match --- apm-agent-api/src/main/java/co/elastic/apm/api/ElasticApm.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apm-agent-api/src/main/java/co/elastic/apm/api/ElasticApm.java b/apm-agent-api/src/main/java/co/elastic/apm/api/ElasticApm.java index 2545f73949..7de78b7493 100644 --- a/apm-agent-api/src/main/java/co/elastic/apm/api/ElasticApm.java +++ b/apm-agent-api/src/main/java/co/elastic/apm/api/ElasticApm.java @@ -279,7 +279,7 @@ public static T getConfig(String key, Class type) { throw new IllegalArgumentException("There is no such option: " + key); } else if (value instanceof IllegalStateException) { return null; - } else if (value.getClass().equals(type)) { + } else if (type.isInstance(value)) { return (T) value; } else { throw new IllegalArgumentException("The option: '"+key+"' is not of type "+type.getName());