From de7f531b6f88cd86f2d97aab853eeb2fed1c2bb2 Mon Sep 17 00:00:00 2001 From: Pablo Collins Date: Wed, 1 Feb 2023 14:55:34 -0500 Subject: [PATCH 1/5] add support for profiler and memory metrics -Dsplunk.profiler.enabled=true -Dsplunk.profiler.memory.enabled=true -Dsplunk.metrics.enabled=true These are turned off by default, so existing users will not be affected at this time. --- instrumentation/Dockerfile | 4 +- instrumentation/README.md | 28 +++- instrumentation/install/instrumentation.conf | 10 +- instrumentation/src/config.c | 62 +++++--- instrumentation/src/config.h | 7 +- instrumentation/src/splunk.c | 34 +++-- instrumentation/src/test_main.c | 136 ++++++++++++------ instrumentation/src/test_main.h | 20 ++- ...tion.conf => instrumentation-default.conf} | 4 +- ...name.conf => instrumentation-options.conf} | 5 +- 10 files changed, 224 insertions(+), 86 deletions(-) rename instrumentation/testdata/{instrumentation.conf => instrumentation-default.conf} (69%) rename instrumentation/testdata/{instrumentation-svcname.conf => instrumentation-options.conf} (74%) diff --git a/instrumentation/Dockerfile b/instrumentation/Dockerfile index 9a8dca8e99..42f17fa504 100644 --- a/instrumentation/Dockerfile +++ b/instrumentation/Dockerfile @@ -7,7 +7,7 @@ RUN apt-get update && \ WORKDIR /libsplunk COPY src /libsplunk/src -COPY testdata/instrumentation.conf /libsplunk/testdata/instrumentation.conf -COPY testdata/instrumentation-svcname.conf /libsplunk/testdata/instrumentation-svcname.conf +COPY testdata/instrumentation-default.conf /libsplunk/testdata/instrumentation-default.conf +COPY testdata/instrumentation-options.conf /libsplunk/testdata/instrumentation-options.conf COPY install/instrumentation.conf /libsplunk/install/instrumentation.conf COPY Makefile /libsplunk/Makefile diff --git a/instrumentation/README.md b/instrumentation/README.md index da16d818ae..395e686cba 100644 --- a/instrumentation/README.md +++ b/instrumentation/README.md @@ -58,6 +58,31 @@ Java instrumentation jar. Typically, it will be set to something like: to set the deployment environment for the Splunk backend. +### disable_telemetry (optional) + +Set this value to `true` to disable the preloader from sending the `splunk.linux-autoinstr.executions` metric to the +local collector. Default: `false`. + +### generate_service_name (optional) + +Set this value to `false` to prevent the preloader from setting the `OTEL_SERVICE_NAME` environment variable. +Default: `true`. + +### enable_profiler (optional) + +Set this value to `true` to pass `-Dsplunk.profiler.enabled=true` to the starting Java executable, which will enable +[AlwaysOn CPU Profiling](https://docs.splunk.com/Observability/apm/profiling/get-data-in-profiling.html). Default: `false`. + +### enable_profiler_memory (optional) + +Set this value to `true` to pass `-Dsplunk.profiler.memory.enabled=true` to the starting Java executable, which will enable +[AlwaysOn Memory Profiling](https://docs.splunk.com/Observability/apm/profiling/get-data-in-profiling.html). Default: `false`. + +### enable_metrics (optional) + +Set this value to `true` to pass `-Dsplunk.metrics.enabled=true` to the starting Java executable, which will enable +[exporting metrics](https://github.com/signalfx/splunk-otel-java/blob/main/docs/metrics.md). Default: `false`. + ### Syntax To add a comment or comment out a line, start it with a `#`. @@ -79,7 +104,8 @@ This environment variable contains a `-javaagent` flag set to the full path of t e.g. `JAVA_TOOL_OPTIONS='-javaagent:/usr/lib/splunk-instrumentation/splunk-otel-javaagent.jar'` -This variable is populated by the .so by concatenating the `java_agent_jar` attribute the config to a `-javaagent:` prefix. +This variable is populated by the .so by concatenating the `java_agent_jar` attribute in the config to a `-javaagent:` +prefix, and then appending any additional system properties specified in the configuration file. #### OTEL_SERVICE_NAME diff --git a/instrumentation/install/instrumentation.conf b/instrumentation/install/instrumentation.conf index 5f125193c5..9c4631f816 100644 --- a/instrumentation/install/instrumentation.conf +++ b/instrumentation/install/instrumentation.conf @@ -1,2 +1,10 @@ -# service_name=default.service java_agent_jar=/usr/lib/splunk-instrumentation/splunk-otel-javaagent.jar +#resource_attributes=deployment.environment=my.environment +#service_name=hardcoded.service + +# note: any of the the following lines may be uncommented to override defaults +#disable_telemetry=true +#generate_service_name=false +#enable_profiler=true +#enable_profiler_memory=true +#enable_metrics=true diff --git a/instrumentation/src/config.c b/instrumentation/src/config.c index bd70423a9d..a665be002c 100644 --- a/instrumentation/src/config.c +++ b/instrumentation/src/config.c @@ -15,23 +15,28 @@ void read_lines(struct config *cfg, FILE *fp); void split_on_eq(char *string, struct kv *pair); +void log_config_field(logger log, const char *field, const char *value); + void load_config(logger log, struct config *cfg, char *file_name) { read_config_file(log, cfg, file_name); - if (cfg->service_name == NULL) { - log_debug(log, "service_name not specified in config"); - } - if (cfg->java_agent_jar == NULL) { - log_debug(log, "java_agent_jar not specified in config"); - } - if (cfg->resource_attributes == NULL) { - log_debug(log, "resource_attributes not specified in config"); - } - if (cfg->disable_telemetry == NULL) { - log_debug(log, "disable_telemetry not specified in config"); - } - if (cfg->generate_service_name == NULL) { - log_debug(log, "generate_service_name not specified in config"); + log_config_field(log, "service_name", cfg->service_name); + log_config_field(log, "java_agent_jar", cfg->java_agent_jar); + log_config_field(log, "resource_attributes", cfg->resource_attributes); + log_config_field(log, "disable_telemetry", cfg->disable_telemetry); + log_config_field(log, "generate_service_name", cfg->generate_service_name); + log_config_field(log, "enable_profiler", cfg->enable_profiler); + log_config_field(log, "enable_profiler_memory", cfg->enable_profiler_memory); + log_config_field(log, "enable_metrics", cfg->enable_metrics); +} + +void log_config_field(logger log, const char *field, const char *value) { + char msg[MAX_LOG_LINE_LEN] = ""; + if (value == NULL) { + snprintf(msg, MAX_LOG_LINE_LEN, "config: %s not specified", field); + } else { + snprintf(msg, MAX_LOG_LINE_LEN, "config: %s=%s", field, value); } + log_debug(log, msg); } void read_config_file(logger log, struct config *cfg, char *file_name) { @@ -68,6 +73,12 @@ void read_lines(struct config *cfg, FILE *fp) { cfg->disable_telemetry = strdup(pair.v); } else if (streq(pair.k, "generate_service_name")) { cfg->generate_service_name = strdup(pair.v); + } else if (streq(pair.k, "enable_profiler")) { + cfg->enable_profiler = strdup(pair.v); + } else if (streq(pair.k, "enable_profiler_memory")) { + cfg->enable_profiler_memory = strdup(pair.v); + } else if (streq(pair.k, "enable_metrics")) { + cfg->enable_metrics = strdup(pair.v); } } } @@ -77,12 +88,14 @@ void split_on_eq(char *string, struct kv *pair) { pair->v = string; } -bool str_eq_true(char *v) { - return v != NULL && !streq("false", v) && !streq("FALSE", v) && !streq("0", v); -} - -bool str_eq_false(char *v) { - return v != NULL && (streq("false", v) || streq("FALSE", v) || streq("0", v)); +bool str_to_bool(char *v, bool defaultVal) { + if (v == NULL) { + return defaultVal; + } + if (streq("false", v) || streq("FALSE", v) || streq("0", v)) { + return false; + } + return true; } void free_config(struct config *cfg) { @@ -101,4 +114,13 @@ void free_config(struct config *cfg) { if (cfg->generate_service_name != NULL) { free(cfg->generate_service_name); } + if (cfg->enable_profiler != NULL) { + free(cfg->enable_profiler); + } + if (cfg->enable_profiler_memory != NULL) { + free(cfg->enable_profiler_memory); + } + if (cfg->enable_metrics != NULL) { + free(cfg->enable_metrics); + } } diff --git a/instrumentation/src/config.h b/instrumentation/src/config.h index 235fd248a5..05cd39fe2e 100644 --- a/instrumentation/src/config.h +++ b/instrumentation/src/config.h @@ -11,13 +11,14 @@ struct config { char *resource_attributes; char *disable_telemetry; char *generate_service_name; + char *enable_profiler; + char *enable_profiler_memory; + char *enable_metrics; }; void load_config(logger log, struct config *cfg, char *file_name); -bool str_eq_true(char *v); - -bool str_eq_false(char *v); +bool str_to_bool(char *v, bool); void free_config(struct config *cfg); diff --git a/instrumentation/src/splunk.c b/instrumentation/src/splunk.c index b3965b83a9..391cf72c97 100644 --- a/instrumentation/src/splunk.c +++ b/instrumentation/src/splunk.c @@ -8,7 +8,7 @@ #include #include -#define ENV_VAR_LEN 512 +#define MAX_ENV_VAR_LEN 512 #define MAX_CONFIG_ATTR_LEN 256 #define MAX_CMDLINE_LEN 16000 #define MAX_ARGS 256 @@ -16,6 +16,9 @@ #define JAVA_TOOL_OPTIONS_PREFIX "-javaagent:"; static char *const conf_file = "/usr/lib/splunk-instrumentation/instrumentation.conf"; +static char *const prof_enabled_cmdline_switch = " -Dsplunk.profiler.enabled=true"; +static char *const prof_memory_enabled_cmdline_switch = " -Dsplunk.profiler.memory.enabled=true"; +static char *const metrics_enabled_cmdline_switch = " -Dsplunk.metrics.enabled=true"; extern char *program_invocation_short_name; @@ -73,7 +76,11 @@ void auto_instrument( .java_agent_jar = NULL, .resource_attributes = NULL, .service_name = NULL, - .disable_telemetry = NULL + .disable_telemetry = NULL, + .generate_service_name = NULL, + .enable_profiler = NULL, + .enable_profiler_memory = NULL, + .enable_metrics = NULL }; load_config_func(log, &cfg, conf_file); if (cfg.java_agent_jar == NULL) { @@ -87,22 +94,22 @@ void auto_instrument( } char service_name[MAX_CMDLINE_LEN] = ""; - if (str_eq_false(cfg.generate_service_name)) { - log_debug(log, "service name generation disabled"); - } else { + if (str_to_bool(cfg.generate_service_name, true)) { get_service_name(log, cr, &cfg, service_name); if (strlen(service_name) == 0) { log_info(log, "service name empty, quitting"); return; } set_env_var(log, otel_service_name_var, service_name); + } else { + log_debug(log, "service name generation explicitly disabled"); } set_java_tool_options(log, &cfg); set_env_var_from_attr(log, "resource_attributes", resource_attributes_var, cfg.resource_attributes); - if (str_eq_true(cfg.disable_telemetry)) { + if (str_to_bool(cfg.disable_telemetry, false)) { log_info(log, "disabling telemetry as per config"); } else { send_otlp_metric_func(log, service_name); @@ -148,7 +155,7 @@ void set_env_var(logger log, const char *var_name, const char *value) { } void set_java_tool_options(logger log, struct config *cfg) { - char java_tool_options[ENV_VAR_LEN] = JAVA_TOOL_OPTIONS_PREFIX; + char java_tool_options[MAX_ENV_VAR_LEN] = JAVA_TOOL_OPTIONS_PREFIX; char log_line[MAX_LOG_LINE_LEN] = ""; size_t jar_path_len = strlen(cfg->java_agent_jar); if (jar_path_len > MAX_CONFIG_ATTR_LEN) { @@ -156,7 +163,18 @@ void set_java_tool_options(logger log, struct config *cfg) { log_warning(log, log_line); return; } - strcat(java_tool_options, (*cfg).java_agent_jar); + strncat(java_tool_options, cfg->java_agent_jar, MAX_ENV_VAR_LEN - strlen(java_tool_options) - 1); + + if (str_to_bool(cfg->enable_profiler, false)) { + strncat(java_tool_options, prof_enabled_cmdline_switch, MAX_ENV_VAR_LEN - strlen(java_tool_options) - 1); + } + if (str_to_bool(cfg->enable_profiler_memory, false)) { + strncat(java_tool_options, prof_memory_enabled_cmdline_switch, MAX_ENV_VAR_LEN - strlen(java_tool_options) - 1); + } + if (str_to_bool(cfg->enable_metrics, false)) { + strncat(java_tool_options, metrics_enabled_cmdline_switch, MAX_ENV_VAR_LEN - strlen(java_tool_options) - 1); + } + sprintf(log_line, "setting JAVA_TOOL_OPTIONS='%s'", java_tool_options); log_debug(log, log_line); setenv(java_tool_options_var, java_tool_options, 0); diff --git a/instrumentation/src/test_main.c b/instrumentation/src/test_main.c index 3cc69e81bb..7a0e294ac6 100644 --- a/instrumentation/src/test_main.c +++ b/instrumentation/src/test_main.c @@ -7,8 +7,8 @@ #include #include -static char *const test_config_path = "testdata/instrumentation.conf"; -static char *const test_config_path_svcname = "testdata/instrumentation-svcname.conf"; +static char *const test_config_path = "testdata/instrumentation-default.conf"; +static char *const test_config_path_all_options = "testdata/instrumentation-options.conf"; int main(void) { run_tests(); @@ -28,8 +28,8 @@ void run_tests() { test_auto_instrument_splunk_env_var_false, test_auto_instrument_splunk_env_var_false_caps, test_auto_instrument_splunk_env_var_zero, - test_read_config, - test_read_config_svcname, + test_read_config_default, + test_read_config_all_options, test_read_config_missing_file, test_read_args_simple, test_read_args_max_args_limit, @@ -55,10 +55,12 @@ void run_tests() { test_is_legal_java_fq_main_class, test_is_legal_java_module_main_class, test_is_legal_module, - test_eq_true, - test_eq_false, + test_str_to_bool, test_disable_telemetry, - test_enable_telemetry + test_enable_telemetry, + test_enable_profiling, + test_enable_profiling_memory, + test_enable_metrics }; for (int i = 0; i < sizeof tests / sizeof tests[0]; ++i) { run_test(tests[i]); @@ -113,7 +115,7 @@ void test_auto_instrument_gen_svc_name_explicitly_disabled(logger l) { auto_instrument(l, access_check_true, "java", fake_config_generate_svcname_disabled, cr, fake_send_otlp_metric); char *logs[256]; int n = get_logs(l, logs); - require_equal_strings(funcname, "service name generation disabled", logs[0]); + require_equal_strings(funcname, "service name generation explicitly disabled", logs[0]); require_equal_strings(funcname, "setting JAVA_TOOL_OPTIONS='-javaagent:/foo/asdf.jar'", logs[1]); require_equal_strings(funcname, "sending metric", logs[2]); require_equal_ints(funcname, 3, n); @@ -195,36 +197,58 @@ void test_auto_instrument_splunk_env_var_zero(logger l) { cmdline_reader_close(cr); } -void test_read_config(logger l) { +void test_read_config_default(logger l) { struct config cfg = {.java_agent_jar = NULL, .service_name = NULL}; load_config(l, &cfg, test_config_path); char *logs[256]; int n = get_logs(l, logs); - char *funcname = "test_read_config"; - require_equal_ints(funcname, 2, n); - require_equal_strings(funcname, "reading config file: testdata/instrumentation.conf", logs[0]); - require_equal_strings(funcname, "generate_service_name not specified in config", logs[1]); - require_equal_strings(funcname, "default.service", cfg.service_name); + char *funcname = "test_read_config_default"; + require_equal_ints(funcname, 9, n); + require_equal_strings(funcname, "reading config file: testdata/instrumentation-default.conf", logs[0]); + require_equal_strings(funcname, "config: service_name not specified", logs[1]); + require_equal_strings(funcname, "config: java_agent_jar=/usr/lib/splunk-instrumentation/splunk-otel-javaagent.jar", logs[2]); + require_equal_strings(funcname, "config: resource_attributes=deployment.environment=test", logs[3]); + require_equal_strings(funcname, "config: disable_telemetry not specified", logs[4]); + require_equal_strings(funcname, "config: generate_service_name not specified", logs[5]); + require_equal_strings(funcname, "config: enable_profiler not specified", logs[6]); + require_equal_strings(funcname, "config: enable_profiler_memory not specified", logs[7]); + require_equal_strings(funcname, "config: enable_metrics not specified", logs[8]); require_equal_strings(funcname, "/usr/lib/splunk-instrumentation/splunk-otel-javaagent.jar", cfg.java_agent_jar); + require_equal_strings(funcname, NULL, cfg.service_name); require_equal_strings(funcname, "deployment.environment=test", cfg.resource_attributes); - require_equal_strings(funcname, "true", cfg.disable_telemetry); + require_equal_strings(funcname, NULL, cfg.disable_telemetry); require_equal_strings(funcname, NULL, cfg.generate_service_name); + require_equal_strings(funcname, NULL, cfg.enable_profiler); + require_equal_strings(funcname, NULL, cfg.enable_profiler_memory); + require_equal_strings(funcname, NULL, cfg.enable_metrics); free_config(&cfg); } -void test_read_config_svcname(logger l) { +void test_read_config_all_options(logger l) { struct config cfg = {.java_agent_jar = NULL, .service_name = NULL}; - load_config(l, &cfg, test_config_path_svcname); + load_config(l, &cfg, test_config_path_all_options); char *logs[256]; int n = get_logs(l, logs); - char *funcname = "test_read_config_svcname"; - require_equal_ints(funcname, 1, n); - require_equal_strings(funcname, "reading config file: testdata/instrumentation-svcname.conf", logs[0]); + char *funcname = "test_read_config_all_options"; + require_equal_ints(funcname, 9, n); + require_equal_strings(funcname, "reading config file: testdata/instrumentation-options.conf", logs[0]); + require_equal_strings(funcname, "config: service_name=default.service", logs[1]); + require_equal_strings(funcname, "config: java_agent_jar=/usr/lib/splunk-instrumentation/splunk-otel-javaagent.jar", logs[2]); + require_equal_strings(funcname, "config: resource_attributes=deployment.environment=test", logs[3]); + require_equal_strings(funcname, "config: disable_telemetry=true", logs[4]); + require_equal_strings(funcname, "config: generate_service_name=true", logs[5]); + require_equal_strings(funcname, "config: enable_profiler=true", logs[6]); + require_equal_strings(funcname, "config: enable_profiler_memory=true", logs[7]); + require_equal_strings(funcname, "config: enable_metrics=true", logs[8]); + require_equal_strings(funcname, "default.service", cfg.service_name); require_equal_strings(funcname, "/usr/lib/splunk-instrumentation/splunk-otel-javaagent.jar", cfg.java_agent_jar); require_equal_strings(funcname, "deployment.environment=test", cfg.resource_attributes); require_equal_strings(funcname, "true", cfg.disable_telemetry); require_equal_strings(funcname, "true", cfg.generate_service_name); + require_equal_strings(funcname, "true", cfg.enable_profiler); + require_equal_strings(funcname, "true", cfg.enable_profiler_memory); + require_equal_strings(funcname, "true", cfg.enable_metrics); free_config(&cfg); } @@ -234,13 +258,16 @@ void test_read_config_missing_file(logger l) { char *logs[256]; int n = get_logs(l, logs); char *funcname = "test_read_config_missing_file"; - require_equal_ints(funcname, 6, n); + require_equal_ints(funcname, 9, n); require_equal_strings(funcname, "file not found: foo.txt", logs[0]); - require_equal_strings(funcname, "service_name not specified in config", logs[1]); - require_equal_strings(funcname, "java_agent_jar not specified in config", logs[2]); - require_equal_strings(funcname, "resource_attributes not specified in config", logs[3]); - require_equal_strings(funcname, "disable_telemetry not specified in config", logs[4]); - require_equal_strings(funcname, "generate_service_name not specified in config", logs[5]); + require_equal_strings(funcname, "config: service_name not specified", logs[1]); + require_equal_strings(funcname, "config: java_agent_jar not specified", logs[2]); + require_equal_strings(funcname, "config: resource_attributes not specified", logs[3]); + require_equal_strings(funcname, "config: disable_telemetry not specified", logs[4]); + require_equal_strings(funcname, "config: generate_service_name not specified", logs[5]); + require_equal_strings(funcname, "config: enable_profiler not specified", logs[6]); + require_equal_strings(funcname, "config: enable_profiler_memory not specified", logs[7]); + require_equal_strings(funcname, "config: enable_metrics not specified", logs[8]); require_equal_strings(funcname, NULL, cfg.service_name); require_equal_strings(funcname, NULL, cfg.java_agent_jar); free_config(&cfg); @@ -499,22 +526,14 @@ void test_is_legal_module(logger l) { require_false(funcname, is_legal_module("foo bar")); } -void test_eq_true(logger l) { - require_true("test_eq_true", str_eq_true("true")); - require_true("test_eq_true", str_eq_true("1")); - require_true("test_eq_true", str_eq_true("TRUE")); - require_false("test_eq_true", str_eq_true("false")); - require_false("test_eq_true", str_eq_true("0")); - require_false("test_eq_true", str_eq_true(NULL)); -} - -void test_eq_false(logger l) { - require_true("str_eq_false", str_eq_false("false")); - require_true("str_eq_false", str_eq_false("FALSE")); - require_true("str_eq_false", str_eq_false("0")); - require_false("str_eq_false", str_eq_false("true")); - require_false("str_eq_false", str_eq_false("TRUE")); - require_false("str_eq_false", str_eq_false("1")); +void test_str_to_bool(logger l) { + require_false("test_str_bool", str_to_bool("false", false)); + require_false("test_str_bool", str_to_bool("FALSE", false)); + require_false("test_str_bool", str_to_bool("0", false)); + require_false("test_str_bool", str_to_bool(NULL, false)); + require_true("test_str_bool", str_to_bool("true", false)); + require_true("test_str_bool", str_to_bool("42", false)); + require_true("test_str_bool", str_to_bool(NULL, true)); } void test_enable_telemetry(logger l) { @@ -534,6 +553,24 @@ void test_disable_telemetry(logger l) { require_equal_strings("test_disable_telemetry", "disabling telemetry as per config", logs[2]); } +void test_enable_profiling(logger l) { + cmdline_reader cr = new_default_test_cmdline_reader(); + auto_instrument(l, access_check_true, "java", fake_config_enable_profiler, cr, fake_send_otlp_metric); + require_env("test_enable_profiling", "-javaagent:/foo/asdf.jar -Dsplunk.profiler.enabled=true", "JAVA_TOOL_OPTIONS"); +} + +void test_enable_profiling_memory(logger l) { + cmdline_reader cr = new_default_test_cmdline_reader(); + auto_instrument(l, access_check_true, "java", fake_config_enable_profiler_memory, cr, fake_send_otlp_metric); + require_env("test_enable_profiling_memory", "-javaagent:/foo/asdf.jar -Dsplunk.profiler.memory.enabled=true", "JAVA_TOOL_OPTIONS"); +} + +void test_enable_metrics(logger l) { + cmdline_reader cr = new_default_test_cmdline_reader(); + auto_instrument(l, access_check_true, "java", fake_config_enable_metrics, cr, fake_send_otlp_metric); + require_env("test_enable_metrics", "-javaagent:/foo/asdf.jar -Dsplunk.metrics.enabled=true", "JAVA_TOOL_OPTIONS"); +} + // fakes/testdata void fake_send_otlp_metric(logger log, char *service_name) { @@ -574,6 +611,21 @@ void fake_config_disable_telemetry_true(logger log, struct config *cfg, char *pa cfg->disable_telemetry = strdup("true"); } +void fake_config_enable_profiler(logger log, struct config *cfg, char *path) { + cfg->java_agent_jar = strdup("/foo/asdf.jar"); + cfg->enable_profiler = strdup("true"); +} + +void fake_config_enable_profiler_memory(logger log, struct config *cfg, char *path) { + cfg->java_agent_jar = strdup("/foo/asdf.jar"); + cfg->enable_profiler_memory = strdup("true"); +} + +void fake_config_enable_metrics(logger log, struct config *cfg, char *path) { + cfg->java_agent_jar = strdup("/foo/asdf.jar"); + cfg->enable_metrics = strdup("true"); +} + cmdline_reader new_default_test_cmdline_reader() { char cmdline[] = {'j', 'a', 'v', 'a', 0, '-', 'j', 'a', 'r', 0, 'f', 'o', 'o', '.', 'j', 'a', 'r', 0}; return new_test_cmdline_reader(cmdline, sizeof(cmdline)); diff --git a/instrumentation/src/test_main.h b/instrumentation/src/test_main.h index 6973369d62..97a5cd5bbe 100644 --- a/instrumentation/src/test_main.h +++ b/instrumentation/src/test_main.h @@ -32,9 +32,9 @@ void test_auto_instrument_splunk_env_var_false_caps(logger l); void test_auto_instrument_splunk_env_var_zero(logger l); -void test_read_config(logger l); +void test_read_config_default(logger l); -void test_read_config_svcname(logger l); +void test_read_config_all_options(logger l); void test_read_config_missing_file(logger l); @@ -86,14 +86,18 @@ void test_is_legal_java_package_element(logger l); void test_is_legal_module(logger l); -void test_eq_true(logger l); - -void test_eq_false(logger l); +void test_str_to_bool(logger l); void test_enable_telemetry(logger l); void test_disable_telemetry(logger l); +void test_enable_profiling(logger l); + +void test_enable_profiling_memory(logger l); + +void test_enable_metrics(logger l); + // fakes/testdata void fake_send_otlp_metric(logger log, char *service_name); @@ -110,6 +114,12 @@ void fake_config_disable_telemetry_not_specified(logger log, struct config *cfg, void fake_config_disable_telemetry_true(logger log, struct config *cfg, char *path); +void fake_config_enable_profiler(logger log, struct config *cfg, char *path); + +void fake_config_enable_profiler_memory(logger log, struct config *cfg, char *path); + +void fake_config_enable_metrics(logger log, struct config *cfg, char *path); + cmdline_reader new_default_test_cmdline_reader(); bool access_check_true(const char *s); diff --git a/instrumentation/testdata/instrumentation.conf b/instrumentation/testdata/instrumentation-default.conf similarity index 69% rename from instrumentation/testdata/instrumentation.conf rename to instrumentation/testdata/instrumentation-default.conf index f1ed49426e..a800bad4f4 100644 --- a/instrumentation/testdata/instrumentation.conf +++ b/instrumentation/testdata/instrumentation-default.conf @@ -1,4 +1,2 @@ -service_name=default.service -java_agent_jar=/usr/lib/splunk-instrumentation/splunk-otel-javaagent.jar resource_attributes=deployment.environment=test -disable_telemetry=true +java_agent_jar=/usr/lib/splunk-instrumentation/splunk-otel-javaagent.jar diff --git a/instrumentation/testdata/instrumentation-svcname.conf b/instrumentation/testdata/instrumentation-options.conf similarity index 74% rename from instrumentation/testdata/instrumentation-svcname.conf rename to instrumentation/testdata/instrumentation-options.conf index 551dfd2323..aa74a6262b 100644 --- a/instrumentation/testdata/instrumentation-svcname.conf +++ b/instrumentation/testdata/instrumentation-options.conf @@ -1,5 +1,8 @@ -service_name=default.service java_agent_jar=/usr/lib/splunk-instrumentation/splunk-otel-javaagent.jar +service_name=default.service resource_attributes=deployment.environment=test disable_telemetry=true generate_service_name=true +enable_profiler=true +enable_profiler_memory=true +enable_metrics=true From 56b2ffa86d80ebb204c8cd49390b4ce6f1390237 Mon Sep 17 00:00:00 2001 From: Pablo Collins Date: Mon, 13 Feb 2023 09:45:41 -0500 Subject: [PATCH 2/5] replace bool with int (osx dev friendly) --- instrumentation/src/args.c | 50 +++++++++++------------ instrumentation/src/args.h | 14 +++---- instrumentation/src/cmdline_reader.c | 2 +- instrumentation/src/cmdline_reader.h | 4 +- instrumentation/src/cmdline_reader_test.c | 2 +- instrumentation/src/config.c | 6 +-- instrumentation/src/config.h | 4 +- instrumentation/src/metrics_client.c | 29 +++++++------ instrumentation/src/metrics_client.h | 2 - instrumentation/src/splunk.c | 28 ++++++------- instrumentation/src/splunk.h | 5 +-- instrumentation/src/test_main.c | 22 +++++----- instrumentation/src/test_main.h | 5 +-- instrumentation/src/test_utils.c | 4 +- instrumentation/src/test_utils.h | 6 +-- 15 files changed, 86 insertions(+), 97 deletions(-) diff --git a/instrumentation/src/args.c b/instrumentation/src/args.c index 286f2c59b9..d18e97b8a1 100644 --- a/instrumentation/src/args.c +++ b/instrumentation/src/args.c @@ -67,14 +67,14 @@ void add_token(struct tokenset *tks, char *token) { } } -bool has_token(struct tokenset *tks, char *token) { +int has_token(struct tokenset *tks, char *token) { // not doing a set implementation at this time since size of array is small for (int i = 0; i < tks->i; ++i) { if (strcmp(tks->tokens[i], token) == 0) { - return true; + return 1; } } - return false; + return 0; } void generate_servicename_from_args(char *dest, char **args, int num_args) { @@ -154,9 +154,9 @@ void dedupe_hyphenated(char *out, char *str, struct tokenset *pTokenset) { } } -bool is_unique_path_element(char *path_element) { +int is_unique_path_element(char *path_element) { if (strlen(path_element) == 0) { - return false; + return 0; } static const char *standard_path_parts[] = {"usr", "local", "bin", "home", "etc", "lib", "opt", ".."}; @@ -164,10 +164,10 @@ bool is_unique_path_element(char *path_element) { for (int i = 0; i < num_standard_path_parts; ++i) { const char *part = standard_path_parts[i]; if (strcmp(part, path_element) == 0) { - return false; + return 0; } } - return true; + return 1; } // removes a .jar suffix/extension from a string if it's long enough @@ -181,7 +181,7 @@ void truncate_extension(char *str) { } } -bool is_legal_java_main_class_with_module(const char *str) { +int is_legal_java_main_class_with_module(const char *str) { int num_slashes = 0; int num_dots = 0; for (int i = 0; str[i] != 0; ++i) { @@ -193,73 +193,73 @@ bool is_legal_java_main_class_with_module(const char *str) { } } if (num_dots == 0) { - return false; + return 0; } if (num_slashes == 0) { return is_legal_java_main_class(str); } else if (num_slashes > 1) { - return false; + return 0; } char *fq_main_package = strdup(str); char *module = strsep(&fq_main_package, "/"); if (!is_legal_java_main_class(fq_main_package)) { - return false; + return 0; } if (!is_legal_module(module)) { - return false; + return 0; } - return true; + return 1; } -bool is_legal_java_main_class(const char *str) { +int is_legal_java_main_class(const char *str) { if (strstr(str, ".") == NULL) { - return false; + return 0; } char *dup = strdup(str); char *prev; - while (true) { + while (1) { char *token = strsep(&dup, "."); if (token == NULL) { return is_capital_letter(prev[0]); } if (!is_legal_java_package_element(token)) { - return false; + return 0; } prev = token; } } -bool is_capital_letter(const char ch) { +int is_capital_letter(const char ch) { return ch >= 'A' && ch <= 'Z'; } -bool is_legal_module(char *module) { +int is_legal_module(char *module) { char *dup = strdup(module); char *token; while ((token = strsep(&dup, ".")) != NULL) { if (!is_legal_java_package_element(token)) { - return false; + return 0; } } - return true; + return 1; } // tests if the parts between the dots in e.g. some.package.MyMain are legal -bool is_legal_java_package_element(const char *str) { +int is_legal_java_package_element(const char *str) { for (int i = 0;; ++i) { char ch = str[i]; if (ch == 0) { break; } if (i == 0 && ch >= '0' && ch <= '9') { - return false; + return 0; } if (ch < '0' || (ch > '9' && ch < 'A') || (ch > 'Z' && ch < '_') || ch == '`' || ch > 'z') { - return false; + return 0; } } - return true; + return 1; } diff --git a/instrumentation/src/args.h b/instrumentation/src/args.h index 4ce56838d1..9e0f5557d7 100644 --- a/instrumentation/src/args.h +++ b/instrumentation/src/args.h @@ -15,7 +15,7 @@ void init_tokenset(struct tokenset *tks); void free_tokenset(struct tokenset *tks); -bool has_token(struct tokenset *tks, char *token); +int has_token(struct tokenset *tks, char *token); void add_token(struct tokenset *tks, char *token); @@ -25,15 +25,15 @@ void free_cmdline_args(char **args, int num_args); void generate_servicename_from_args(char *dest, char **args, int num_args); -bool is_legal_java_main_class(const char *str); +int is_legal_java_main_class(const char *str); -bool is_capital_letter(char ch); +int is_capital_letter(char ch); -bool is_legal_java_main_class_with_module(const char *str); +int is_legal_java_main_class_with_module(const char *str); -bool is_legal_module(char *module); +int is_legal_module(char *module); -bool is_legal_java_package_element(const char *str); +int is_legal_java_package_element(const char *str); void transform_multi_jars(char *dest, char *arg, struct tokenset *tks); @@ -41,7 +41,7 @@ void transform_jar_path_elements(char *out, char *path); void dedupe_hyphenated(char *out, char *str, struct tokenset *pTokenset); -bool is_unique_path_element(char *path_element); +int is_unique_path_element(char *path_element); void truncate_extension(char *str); diff --git a/instrumentation/src/cmdline_reader.c b/instrumentation/src/cmdline_reader.c index 19dd4f1420..8255ef4fff 100644 --- a/instrumentation/src/cmdline_reader.c +++ b/instrumentation/src/cmdline_reader.c @@ -22,7 +22,7 @@ void cmdline_reader_open(cmdline_reader cr) { cr->f = fopen(fname, "r"); } -bool cmdline_reader_is_eof(cmdline_reader cr) { +int cmdline_reader_is_eof(cmdline_reader cr) { return feof(cr->f) != 0; } diff --git a/instrumentation/src/cmdline_reader.h b/instrumentation/src/cmdline_reader.h index 54f518830a..2aba64dd01 100644 --- a/instrumentation/src/cmdline_reader.h +++ b/instrumentation/src/cmdline_reader.h @@ -1,13 +1,11 @@ #ifndef INSTRUMENTATION_CMDLINE_READER_H #define INSTRUMENTATION_CMDLINE_READER_H -#include - typedef struct cmdline_reader_impl *cmdline_reader; cmdline_reader new_cmdline_reader(); cmdline_reader new_test_cmdline_reader(char *cmdline, int len); void cmdline_reader_open(cmdline_reader cr); -bool cmdline_reader_is_eof(cmdline_reader cr); +int cmdline_reader_is_eof(cmdline_reader cr); char cmdline_reader_get_char(cmdline_reader cr); void cmdline_reader_close(cmdline_reader cr); diff --git a/instrumentation/src/cmdline_reader_test.c b/instrumentation/src/cmdline_reader_test.c index ed1fb20c62..512e4b7c20 100644 --- a/instrumentation/src/cmdline_reader_test.c +++ b/instrumentation/src/cmdline_reader_test.c @@ -31,7 +31,7 @@ cmdline_reader new_test_cmdline_reader(char *cmdline, int size) { void cmdline_reader_open(cmdline_reader cr) { } -bool cmdline_reader_is_eof(cmdline_reader cr) { +int cmdline_reader_is_eof(cmdline_reader cr) { return cr->i >= cr->size; } diff --git a/instrumentation/src/config.c b/instrumentation/src/config.c index a665be002c..c34f00797c 100644 --- a/instrumentation/src/config.c +++ b/instrumentation/src/config.c @@ -88,14 +88,14 @@ void split_on_eq(char *string, struct kv *pair) { pair->v = string; } -bool str_to_bool(char *v, bool defaultVal) { +int str_to_bool(char *v, int defaultVal) { if (v == NULL) { return defaultVal; } if (streq("false", v) || streq("FALSE", v) || streq("0", v)) { - return false; + return 0; } - return true; + return 1; } void free_config(struct config *cfg) { diff --git a/instrumentation/src/config.h b/instrumentation/src/config.h index 05cd39fe2e..c384a11e51 100644 --- a/instrumentation/src/config.h +++ b/instrumentation/src/config.h @@ -1,8 +1,6 @@ #ifndef INSTRUMENTATION_CONFIG_H #define INSTRUMENTATION_CONFIG_H -#include - #include "logger.h" struct config { @@ -18,7 +16,7 @@ struct config { void load_config(logger log, struct config *cfg, char *file_name); -bool str_to_bool(char *v, bool); +int str_to_bool(char *v, int); void free_config(struct config *cfg); diff --git a/instrumentation/src/metrics_client.c b/instrumentation/src/metrics_client.c index 368f3f0be6..3130e0dfc4 100644 --- a/instrumentation/src/metrics_client.c +++ b/instrumentation/src/metrics_client.c @@ -6,7 +6,6 @@ #include #include #include -#include static const int RECV_BUF_LEN = 1024; @@ -14,15 +13,15 @@ static const int METRIC_JSON_MAX_LEN = 1024; static char *const expected = "HTTP/1.1 200 OK"; -bool http_post(char *host, int port, char *method, char *path, char *postData, logger pImpl); +int http_post(char *host, int port, char *method, char *path, char *postData, logger pImpl); int make_socket(int timeout_seconds); -bool connect_http(const char *host, int port, int socket_descriptor); +int connect_http(const char *host, int port, int socket_descriptor); -bool post(int socket_descriptor, char *host, int port, char *method, char *path, char *postData); +int post(int socket_descriptor, char *host, int port, char *method, char *path, char *postData); -bool receive(int socket_descriptor); +int receive(int socket_descriptor); int mk_metrics_json(char *dest, int max_len, char *service_name); @@ -52,29 +51,29 @@ int mk_metrics_json(char *dest, int max_len, char *service_name) { return snprintf(dest, max_len, format, service_name); } -bool http_post(char *host, int port, char *method, char *path, char *postData, logger log) { +int http_post(char *host, int port, char *method, char *path, char *postData, logger log) { int socket_descriptor = make_socket(1); if (socket_descriptor == -1) { log_debug(log, "metrics client failed to open socket"); - return false; + return 0; } if (!connect_http(host, port, socket_descriptor)) { log_debug(log, "metrics client failed to connect"); - return false; + return 0; } if (!post(socket_descriptor, host, port, method, path, postData)) { log_debug(log, "metrics client failed to send"); - return false; + return 0; } if (!receive(socket_descriptor)) { log_debug(log, "metrics client failed to receive response"); - return false; + return 0; } - return true; + return 1; } int make_socket(int timeout_seconds) { @@ -98,7 +97,7 @@ int make_socket(int timeout_seconds) { return socket_descriptor; } -bool connect_http(const char *host, int port, int socket_descriptor) { +int connect_http(const char *host, int port, int socket_descriptor) { struct sockaddr_in address; address.sin_family = AF_INET; address.sin_addr.s_addr = inet_addr(host); @@ -107,7 +106,7 @@ bool connect_http(const char *host, int port, int socket_descriptor) { return errno == 0; } -bool post(int socket_descriptor, char *host, int port, char *method, char *path, char *postData) { +int post(int socket_descriptor, char *host, int port, char *method, char *path, char *postData) { char *req_pattern = "%s %s HTTP/1.1\n" "Host: %s:%d\n" "Content-Type: application/json\n" @@ -122,12 +121,12 @@ bool post(int socket_descriptor, char *host, int port, char *method, char *path, return num_bytes_sent == req_len; } -bool receive(int socket_descriptor) { +int receive(int socket_descriptor) { char buf[RECV_BUF_LEN]; ssize_t recv_size = recv(socket_descriptor, buf, RECV_BUF_LEN, 0); size_t expected_len = strlen(expected); if (recv_size < expected_len) { - return false; + return 0; } return strncmp(expected, buf, expected_len) == 0; diff --git a/instrumentation/src/metrics_client.h b/instrumentation/src/metrics_client.h index 56648f6ac8..e43a348949 100644 --- a/instrumentation/src/metrics_client.h +++ b/instrumentation/src/metrics_client.h @@ -1,8 +1,6 @@ #ifndef INSTRUMENTATION_METRICS_CLIENT_H #define INSTRUMENTATION_METRICS_CLIENT_H -#include - #include "logger.h" typedef void (*send_otlp_metric_func_t)(logger, char *); diff --git a/instrumentation/src/splunk.c b/instrumentation/src/splunk.c index 391cf72c97..67ba326fc5 100644 --- a/instrumentation/src/splunk.c +++ b/instrumentation/src/splunk.c @@ -22,15 +22,15 @@ static char *const metrics_enabled_cmdline_switch = " -Dsplunk.metrics.enabled=t extern char *program_invocation_short_name; -bool has_read_access(const char *s); +int has_read_access(const char *s); void set_java_tool_options(logger log, struct config *cfg); void get_service_name_from_cmdline(logger log, char *dest, cmdline_reader cr); -bool is_disable_env_set(); +int is_disable_env_set(); -bool is_java_tool_options_set(); +int is_java_tool_options_set(); void set_env_var(logger log, const char *var_name, const char *value); @@ -94,7 +94,7 @@ void auto_instrument( } char service_name[MAX_CMDLINE_LEN] = ""; - if (str_to_bool(cfg.generate_service_name, true)) { + if (str_to_bool(cfg.generate_service_name, 1)) { get_service_name(log, cr, &cfg, service_name); if (strlen(service_name) == 0) { log_info(log, "service name empty, quitting"); @@ -109,7 +109,7 @@ void auto_instrument( set_env_var_from_attr(log, "resource_attributes", resource_attributes_var, cfg.resource_attributes); - if (str_to_bool(cfg.disable_telemetry, false)) { + if (str_to_bool(cfg.disable_telemetry, 0)) { log_info(log, "disabling telemetry as per config"); } else { send_otlp_metric_func(log, service_name); @@ -165,13 +165,13 @@ void set_java_tool_options(logger log, struct config *cfg) { } strncat(java_tool_options, cfg->java_agent_jar, MAX_ENV_VAR_LEN - strlen(java_tool_options) - 1); - if (str_to_bool(cfg->enable_profiler, false)) { + if (str_to_bool(cfg->enable_profiler, 0)) { strncat(java_tool_options, prof_enabled_cmdline_switch, MAX_ENV_VAR_LEN - strlen(java_tool_options) - 1); } - if (str_to_bool(cfg->enable_profiler_memory, false)) { + if (str_to_bool(cfg->enable_profiler_memory, 0)) { strncat(java_tool_options, prof_memory_enabled_cmdline_switch, MAX_ENV_VAR_LEN - strlen(java_tool_options) - 1); } - if (str_to_bool(cfg->enable_metrics, false)) { + if (str_to_bool(cfg->enable_metrics, 0)) { strncat(java_tool_options, metrics_enabled_cmdline_switch, MAX_ENV_VAR_LEN - strlen(java_tool_options) - 1); } @@ -180,26 +180,26 @@ void set_java_tool_options(logger log, struct config *cfg) { setenv(java_tool_options_var, java_tool_options, 0); } -bool is_disable_env_set() { +int is_disable_env_set() { char *env = getenv(disable_env_var); return env && !streq("false", env) && !streq("FALSE", env) && !streq("0", env); } -bool is_java_tool_options_set() { +int is_java_tool_options_set() { char *env = getenv(java_tool_options_var); return env != NULL && strlen(env) > 0; } -bool has_read_access(const char *s) { +int has_read_access(const char *s) { return access(s, R_OK) == 0; } -bool streq(const char *expected, const char *actual) { +int streq(const char *expected, const char *actual) { if (expected == NULL && actual == NULL) { - return true; + return 1; } if (expected == NULL || actual == NULL) { - return false; + return 0; } return strcmp(expected, actual) == 0; } diff --git a/instrumentation/src/splunk.h b/instrumentation/src/splunk.h index f3c953edbf..0aa8066d6b 100644 --- a/instrumentation/src/splunk.h +++ b/instrumentation/src/splunk.h @@ -5,14 +5,13 @@ #include "config.h" #include "cmdline_reader.h" #include "metrics_client.h" -#include static char *const disable_env_var = "DISABLE_SPLUNK_AUTOINSTRUMENTATION"; static char *const java_tool_options_var = "JAVA_TOOL_OPTIONS"; static char *const otel_service_name_var = "OTEL_SERVICE_NAME"; static char *const resource_attributes_var = "OTEL_RESOURCE_ATTRIBUTES"; -typedef bool (*has_access_func_t)(const char *); +typedef int (*has_access_func_t)(const char *); typedef void (*load_config_func_t)(logger log, struct config *, char *); @@ -25,6 +24,6 @@ void auto_instrument( send_otlp_metric_func_t send_otlp_metric_func ); -bool streq(const char *expected, const char *actual); +int streq(const char *expected, const char *actual); #endif //SPLUNK_INSTRUMENTATION_SPLUNK_H diff --git a/instrumentation/src/test_main.c b/instrumentation/src/test_main.c index 7a0e294ac6..03c6ce1fcd 100644 --- a/instrumentation/src/test_main.c +++ b/instrumentation/src/test_main.c @@ -527,13 +527,13 @@ void test_is_legal_module(logger l) { } void test_str_to_bool(logger l) { - require_false("test_str_bool", str_to_bool("false", false)); - require_false("test_str_bool", str_to_bool("FALSE", false)); - require_false("test_str_bool", str_to_bool("0", false)); - require_false("test_str_bool", str_to_bool(NULL, false)); - require_true("test_str_bool", str_to_bool("true", false)); - require_true("test_str_bool", str_to_bool("42", false)); - require_true("test_str_bool", str_to_bool(NULL, true)); + require_false("test_str_bool", str_to_bool("false", 0)); + require_false("test_str_bool", str_to_bool("FALSE", 0)); + require_false("test_str_bool", str_to_bool("0", 0)); + require_false("test_str_bool", str_to_bool(NULL, 0)); + require_true("test_str_bool", str_to_bool("true", 0)); + require_true("test_str_bool", str_to_bool("42", 0)); + require_true("test_str_bool", str_to_bool(NULL, 1)); } void test_enable_telemetry(logger l) { @@ -631,12 +631,12 @@ cmdline_reader new_default_test_cmdline_reader() { return new_test_cmdline_reader(cmdline, sizeof(cmdline)); } -bool access_check_true(const char *s) { - return true; +int access_check_true(const char *s) { + return 1; } -bool access_check_false(const char *s) { - return false; +int access_check_false(const char *s) { + return 0; } int tomcat_args(char *args[]) { diff --git a/instrumentation/src/test_main.h b/instrumentation/src/test_main.h index 97a5cd5bbe..12222dc56e 100644 --- a/instrumentation/src/test_main.h +++ b/instrumentation/src/test_main.h @@ -4,7 +4,6 @@ #include "logger.h" #include "config.h" #include "cmdline_reader.h" -#include typedef void (test_func_t)(logger); @@ -122,9 +121,9 @@ void fake_config_enable_metrics(logger log, struct config *cfg, char *path); cmdline_reader new_default_test_cmdline_reader(); -bool access_check_true(const char *s); +int access_check_true(const char *s); -bool access_check_false(const char *s); +int access_check_false(const char *s); int tomcat_args(char *args[]); diff --git a/instrumentation/src/test_utils.c b/instrumentation/src/test_utils.c index ad0a87aa09..a3a5fca27b 100644 --- a/instrumentation/src/test_utils.c +++ b/instrumentation/src/test_utils.c @@ -9,14 +9,14 @@ void print_logs(char **logs, int n) { } } -void require_true(char *funcname, bool actual) { +void require_true(char *funcname, int actual) { if (!actual) { printf("%s: require_true: got false\n", funcname); fail(); } } -void require_false(char *funcname, bool actual) { +void require_false(char *funcname, int actual) { if (actual) { printf("%s: require_false: got true\n", funcname); fail(); diff --git a/instrumentation/src/test_utils.h b/instrumentation/src/test_utils.h index 99071c60c8..ea795b56cd 100644 --- a/instrumentation/src/test_utils.h +++ b/instrumentation/src/test_utils.h @@ -1,13 +1,11 @@ #ifndef INSTRUMENTATION_TEST_UTILS_H #define INSTRUMENTATION_TEST_UTILS_H -#include - void print_logs(char **logs, int n); -void require_true(char *funcname, bool actual); +void require_true(char *funcname, int actual); -void require_false(char *funcname, bool actual); +void require_false(char *funcname, int actual); void require_equal_strings(char *funcname, char *expected, char *actual); From 61e917be863adf9297464dcc1e5b3998f86944e8 Mon Sep 17 00:00:00 2001 From: Pablo Collins Date: Tue, 14 Feb 2023 11:49:01 -0500 Subject: [PATCH 3/5] check for truncation on strncat, update docs, add tests --- instrumentation/README.md | 3 +- instrumentation/src/splunk.c | 46 ++++++++++++++--- instrumentation/src/splunk.h | 2 + instrumentation/src/test_main.c | 84 ++++++++++++++++++++++++++++++-- instrumentation/src/test_main.h | 16 ++++++ instrumentation/src/test_utils.c | 10 ++++ instrumentation/src/test_utils.h | 2 + 7 files changed, 151 insertions(+), 12 deletions(-) diff --git a/instrumentation/README.md b/instrumentation/README.md index 395e686cba..fe2c885e38 100644 --- a/instrumentation/README.md +++ b/instrumentation/README.md @@ -46,7 +46,8 @@ generated by the Java auto-instrumentation library. This is an optional override for the service name that would otherwise be generated by the shared object before Java startup. By default, this line is commented out, but can be uncommented to override the generated name. If this parameter is set, all instrumented Java applications on this host will have the specified service name (via the -OTEL_SERVICE_NAME environment variable). +OTEL_SERVICE_NAME environment variable). If this override is set and `generate_service_name` is also explicitly set to +`false`, that parameter will win, and the service name will not be set. ### resource_attributes (optional -- typically set by the installer script) diff --git a/instrumentation/src/splunk.c b/instrumentation/src/splunk.c index 67ba326fc5..fa456f5248 100644 --- a/instrumentation/src/splunk.c +++ b/instrumentation/src/splunk.c @@ -38,6 +38,8 @@ void set_env_var_from_attr(logger log, const char *attr_name, const char *env_va void get_service_name(logger log, cmdline_reader cr, struct config *cfg, char *dest); +void log_line_length_warning(logger log, char *log_line); + // The entry point for all executables prior to their execution. If the executable is named "java", we // set the env vars JAVA_TOOL_OPTIONS to the path of the java agent jar and OTEL_SERVICE_NAME to the // service name based on the arguments to the java command. @@ -163,23 +165,55 @@ void set_java_tool_options(logger log, struct config *cfg) { log_warning(log, log_line); return; } - strncat(java_tool_options, cfg->java_agent_jar, MAX_ENV_VAR_LEN - strlen(java_tool_options) - 1); - + int remaining = concat_strings(java_tool_options, cfg->java_agent_jar, MAX_ENV_VAR_LEN); + // It's not possible (at time of writing) for `remaining` to be less than zero in this function because the sum of + // MAX_CONFIG_ATTR_LEN (256) and the lengths of the pre-defined command line switches will always be less than + // MAX_ENV_VAR_LEN (512), but check for truncation anyway to account for future additions. + if (remaining < 0) { + log_line_length_warning(log, log_line); + return; + } if (str_to_bool(cfg->enable_profiler, 0)) { - strncat(java_tool_options, prof_enabled_cmdline_switch, MAX_ENV_VAR_LEN - strlen(java_tool_options) - 1); + remaining = concat_strings(java_tool_options, prof_enabled_cmdline_switch, MAX_ENV_VAR_LEN); + if (remaining < 0) { + log_line_length_warning(log, log_line); + return; + } } if (str_to_bool(cfg->enable_profiler_memory, 0)) { - strncat(java_tool_options, prof_memory_enabled_cmdline_switch, MAX_ENV_VAR_LEN - strlen(java_tool_options) - 1); + remaining = concat_strings(java_tool_options, prof_memory_enabled_cmdline_switch, MAX_ENV_VAR_LEN); + if (remaining < 0) { + log_line_length_warning(log, log_line); + return; + } } if (str_to_bool(cfg->enable_metrics, 0)) { - strncat(java_tool_options, metrics_enabled_cmdline_switch, MAX_ENV_VAR_LEN - strlen(java_tool_options) - 1); + remaining = concat_strings(java_tool_options, metrics_enabled_cmdline_switch, MAX_ENV_VAR_LEN); + if (remaining < 0) { + log_line_length_warning(log, log_line); + return; + } } - sprintf(log_line, "setting JAVA_TOOL_OPTIONS='%s'", java_tool_options); log_debug(log, log_line); setenv(java_tool_options_var, java_tool_options, 0); } +void log_line_length_warning(logger log, char *log_line) { + sprintf(log_line, "excessive line length: not setting JAVA_TOOL_OPTIONS"); + log_warning(log, log_line); +} + +// concat_strings concatenates the string defined by src to the memory location pointed to by dest, +// returning the number of characters remaining in the dest buffer. The return value may be negative +// indicating the number of characters that were not concatenated because of a lack of space. The tot_dest_size +// argument indicates the total number of bytes in the dest memory array. See unit tests for examples. +int concat_strings(char *dest, char *src, int tot_dest_size) { + int orig_dest_len = (int) strlen(dest); + strncat(dest, src, tot_dest_size - 1); + return tot_dest_size - (int) orig_dest_len - (int) strlen(src) - 1; +} + int is_disable_env_set() { char *env = getenv(disable_env_var); return env && !streq("false", env) && !streq("FALSE", env) && !streq("0", env); diff --git a/instrumentation/src/splunk.h b/instrumentation/src/splunk.h index 0aa8066d6b..0f17279f86 100644 --- a/instrumentation/src/splunk.h +++ b/instrumentation/src/splunk.h @@ -26,4 +26,6 @@ void auto_instrument( int streq(const char *expected, const char *actual); +int concat_strings(char *dest, char *src, int tot_dest_size); + #endif //SPLUNK_INSTRUMENTATION_SPLUNK_H diff --git a/instrumentation/src/test_main.c b/instrumentation/src/test_main.c index 03c6ce1fcd..4bab4bca75 100644 --- a/instrumentation/src/test_main.c +++ b/instrumentation/src/test_main.c @@ -60,7 +60,13 @@ void run_tests() { test_enable_telemetry, test_enable_profiling, test_enable_profiling_memory, - test_enable_metrics + test_enable_metrics, + test_concat_string_to_empty_just_enough_room, + test_concat_string_to_empty_extra_room, + test_concat_string_to_empty_not_enough_room, + test_concat_string_to_nonempty_just_enough_room, + test_long_cfg_attributes, + test_auto_instrument_gen_svcname_disabled_but_specified }; for (int i = 0; i < sizeof tests / sizeof tests[0]; ++i) { run_test(tests[i]); @@ -122,6 +128,19 @@ void test_auto_instrument_gen_svc_name_explicitly_disabled(logger l) { require_unset_env(funcname, otel_service_name_var); } +void test_auto_instrument_gen_svcname_disabled_but_specified(logger l) { + char *funcname = "test_auto_instrument_gen_svcname_disabled_but_specified"; + cmdline_reader cr = new_default_test_cmdline_reader(); + auto_instrument(l, access_check_true, "java", fake_config_generate_svcname_disabled_but_explicitly_specified, cr, fake_send_otlp_metric); + char *logs[256]; + int n = get_logs(l, logs); + require_equal_strings(funcname, "service name generation explicitly disabled", logs[0]); + require_equal_strings(funcname, "setting JAVA_TOOL_OPTIONS='-javaagent:/foo/asdf.jar'", logs[1]); + require_equal_strings(funcname, "sending metric", logs[2]); + require_equal_ints(funcname, 3, n); + require_unset_env(funcname, otel_service_name_var); +} + void test_auto_instrument_no_svc_name_in_config(logger l) { cmdline_reader cr = new_default_test_cmdline_reader(); auto_instrument(l, access_check_true, "java", fake_config_no_svcname, cr, fake_send_otlp_metric); @@ -191,8 +210,7 @@ void test_auto_instrument_splunk_env_var_false_caps(logger l) { void test_auto_instrument_splunk_env_var_zero(logger l) { setenv(disable_env_var, "0", 0); cmdline_reader cr = new_default_test_cmdline_reader(); - auto_instrument(l, access_check_true, "java", fake_config_svcname_explicitly_specified, cr, - fake_send_otlp_metric); + auto_instrument(l, access_check_true, "java", fake_config_svcname_explicitly_specified, cr, fake_send_otlp_metric); require_env("test_auto_instrument_splunk_env_var_zero", "-javaagent:/foo/asdf.jar", "JAVA_TOOL_OPTIONS"); cmdline_reader_close(cr); } @@ -571,6 +589,46 @@ void test_enable_metrics(logger l) { require_env("test_enable_metrics", "-javaagent:/foo/asdf.jar -Dsplunk.metrics.enabled=true", "JAVA_TOOL_OPTIONS"); } +void test_concat_string_to_empty_just_enough_room(logger l) { + char *funcname = "test_concat_string_to_empty_just_enough_room"; + char dest[4] = ""; + int res = concat_strings(dest, "abc", 4); + require_equal_ints(funcname, 0, res); + require_equal_strings(funcname, "abc", dest); +} + +void test_concat_string_to_empty_extra_room(logger l) { + char *funcname = "test_concat_string_to_empty_extra_room"; + char dest[8] = ""; + int res = concat_strings(dest, "abc", 8); + require_equal_ints(funcname, 4, res); + require_equal_strings(funcname, "abc", dest); +} + +void test_concat_string_to_empty_not_enough_room(logger l) { + char *funcname = "test_concat_string_to_empty_not_enough_room"; + char dest[2] = ""; + int res = concat_strings(dest, "abc", 2); + require_equal_ints(funcname, -2, res); + require_equal_strings(funcname, "a", dest); +} + +void test_concat_string_to_nonempty_just_enough_room(logger l) { + char *funcname = "test_concat_string_to_nonempty_just_enough_room"; + char dest[4] = "ab"; + int res = concat_strings(dest, "c", 4); + require_equal_ints(funcname, 0, res); + require_equal_strings(funcname, "abc", dest); +} + +void test_long_cfg_attributes(logger l) { + cmdline_reader cr = new_default_test_cmdline_reader(); + auto_instrument(l, access_check_true, "java", fake_config_max_attributes, cr, fake_send_otlp_metric); + require_env_len("test_long_cfg_attributes", 255, "OTEL_SERVICE_NAME"); + require_env_len("test_long_cfg_attributes", 365, "JAVA_TOOL_OPTIONS"); + require_env_len("test_long_cfg_attributes", 255, "OTEL_RESOURCE_ATTRIBUTES"); +} + // fakes/testdata void fake_send_otlp_metric(logger log, char *service_name) { @@ -594,7 +652,12 @@ void fake_config_generate_svcname_enabled(logger log, struct config *cfg, char * void fake_config_generate_svcname_disabled(logger log, struct config *cfg, char *path) { cfg->java_agent_jar = strdup("/foo/asdf.jar"); cfg->generate_service_name = strdup("false"); - cfg->disable_telemetry = NULL; +} + +void fake_config_generate_svcname_disabled_but_explicitly_specified(logger log, struct config *cfg, char *path) { + cfg->java_agent_jar = strdup("/foo/asdf.jar"); + cfg->generate_service_name = strdup("false"); + cfg->service_name = strdup("my-explicit-servicename"); } void fake_config_no_svcname(logger log, struct config *cfg, char *path) { @@ -603,7 +666,6 @@ void fake_config_no_svcname(logger log, struct config *cfg, char *path) { void fake_config_disable_telemetry_not_specified(logger log, struct config *cfg, char *path) { cfg->java_agent_jar = strdup("/foo/asdf.jar"); - cfg->disable_telemetry = NULL; } void fake_config_disable_telemetry_true(logger log, struct config *cfg, char *path) { @@ -626,6 +688,18 @@ void fake_config_enable_metrics(logger log, struct config *cfg, char *path) { cfg->enable_metrics = strdup("true"); } +void fake_config_max_attributes(logger log, struct config *cfg, char *path) { + char *str_255 = "1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890" + "1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890" + "1234567890123456789012345678901234567890123456789012345"; + cfg->java_agent_jar = strdup(str_255); + cfg->service_name = strdup(str_255); + cfg->resource_attributes = strdup(str_255); + cfg->enable_profiler = strdup("true"); + cfg->enable_profiler_memory = strdup("true"); + cfg->enable_metrics = strdup("true"); +} + cmdline_reader new_default_test_cmdline_reader() { char cmdline[] = {'j', 'a', 'v', 'a', 0, '-', 'j', 'a', 'r', 0, 'f', 'o', 'o', '.', 'j', 'a', 'r', 0}; return new_test_cmdline_reader(cmdline, sizeof(cmdline)); diff --git a/instrumentation/src/test_main.h b/instrumentation/src/test_main.h index 12222dc56e..f3ad32141a 100644 --- a/instrumentation/src/test_main.h +++ b/instrumentation/src/test_main.h @@ -97,6 +97,18 @@ void test_enable_profiling_memory(logger l); void test_enable_metrics(logger l); +void test_concat_string_to_empty_just_enough_room(logger l); + +void test_concat_string_to_empty_extra_room(logger l); + +void test_concat_string_to_empty_not_enough_room(logger l); + +void test_concat_string_to_nonempty_just_enough_room(logger l); + +void test_long_cfg_attributes(logger l); + +void test_auto_instrument_gen_svcname_disabled_but_specified(logger l); + // fakes/testdata void fake_send_otlp_metric(logger log, char *service_name); @@ -107,6 +119,8 @@ void fake_config_generate_svcname_enabled(logger log, struct config *cfg, char * void fake_config_generate_svcname_disabled(logger log, struct config *cfg, char *path); +void fake_config_generate_svcname_disabled_but_explicitly_specified(logger log, struct config *cfg, char *path); + void fake_config_no_svcname(logger log, struct config *cfg, char *path); void fake_config_disable_telemetry_not_specified(logger log, struct config *cfg, char *path); @@ -119,6 +133,8 @@ void fake_config_enable_profiler_memory(logger log, struct config *cfg, char *pa void fake_config_enable_metrics(logger log, struct config *cfg, char *path); +void fake_config_max_attributes(logger log, struct config *cfg, char *path); + cmdline_reader new_default_test_cmdline_reader(); int access_check_true(const char *s); diff --git a/instrumentation/src/test_utils.c b/instrumentation/src/test_utils.c index a3a5fca27b..bdaf57fd01 100644 --- a/instrumentation/src/test_utils.c +++ b/instrumentation/src/test_utils.c @@ -1,5 +1,6 @@ #include #include +#include #include "test_utils.h" #include "splunk.h" @@ -45,6 +46,15 @@ void require_env(char *funcname, char *expected, char *env_var) { } } +void require_env_len(char *funcname, int expected_len, char *env_var) { + char *env = getenv(env_var); + size_t env_len = strlen(env); + if (env_len != expected_len) { + printf("%s: require_env_len: %s expected len [%d] got [%d]\n", funcname, env_var, expected_len, (int) env_len); + fail(); + } +} + void require_unset_env(char *funcname, char *env_var) { char *env = getenv(env_var); if (env) { diff --git a/instrumentation/src/test_utils.h b/instrumentation/src/test_utils.h index ea795b56cd..50245dd97d 100644 --- a/instrumentation/src/test_utils.h +++ b/instrumentation/src/test_utils.h @@ -13,6 +13,8 @@ void require_equal_ints(char *funcname, int expected, int actual); void require_env(char *funcname, char *expected, char *env_var); +void require_env_len(char *funcname, int expected_len, char *env_var); + void require_unset_env(char *funcname, char *env_var); void fail(); From c5c9fcf104e6d8253c73638a3ccf879bd12e84c1 Mon Sep 17 00:00:00 2001 From: Pablo Collins Date: Wed, 15 Feb 2023 10:10:39 -0500 Subject: [PATCH 4/5] address pr feedback (improve readme) --- instrumentation/README.md | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/instrumentation/README.md b/instrumentation/README.md index fe2c885e38..39ae31e9cd 100644 --- a/instrumentation/README.md +++ b/instrumentation/README.md @@ -38,8 +38,8 @@ The full path to the auto instrumentation JAR (provided by the installer). #### `generate_service_name` (optional) Whether to disable service name generation by the .so. If set to "true" (the default), the .so will attempt to generate -a service name from the Java command arguments. If set to "false", it will not set the service name, leaving it to be -generated by the Java auto-instrumentation library. +a service name from the Java command arguments, or provide a hard-coded service name if `service_name` is set. If set to +"false", it will not set the service name, leaving it to be generated by the Java auto-instrumentation library. #### `service_name` (optional) @@ -67,17 +67,21 @@ local collector. Default: `false`. ### generate_service_name (optional) Set this value to `false` to prevent the preloader from setting the `OTEL_SERVICE_NAME` environment variable. -Default: `true`. +Default: `true`. If this value is `false`, the preloader will not set `OTEL_SERVICE_NAME`, and the soon-to-be running +Java instrumentation library will attempt to set it instead. In the future, this will be the default behavior. ### enable_profiler (optional) Set this value to `true` to pass `-Dsplunk.profiler.enabled=true` to the starting Java executable, which will enable -[AlwaysOn CPU Profiling](https://docs.splunk.com/Observability/apm/profiling/get-data-in-profiling.html). Default: `false`. +[AlwaysOn CPU Profiling](https://docs.splunk.com/Observability/apm/profiling/get-data-in-profiling.html). +Default: `false`. ### enable_profiler_memory (optional) -Set this value to `true` to pass `-Dsplunk.profiler.memory.enabled=true` to the starting Java executable, which will enable -[AlwaysOn Memory Profiling](https://docs.splunk.com/Observability/apm/profiling/get-data-in-profiling.html). Default: `false`. +Set this value to `true` to pass `-Dsplunk.profiler.memory.enabled=true` to the starting Java executable, which will +enable +[AlwaysOn Memory Profiling](https://docs.splunk.com/Observability/apm/profiling/get-data-in-profiling.html). +Default: `false`. ### enable_metrics (optional) @@ -120,7 +124,7 @@ _Meta: link to docs about how service name is used and why it's required._ #### OTEL_RESOURCE_ATTRIBUTES This environment variable contains a list of name-value pairs (separated by `=`s) passed on to the Java instrumentation -jar. +jar. This variable is set directly from the optional `resource_attributes` attribute in the config. From b4beb1a1de60d0f5348a08666834c53296ab56ed Mon Sep 17 00:00:00 2001 From: Pablo Collins Date: Wed, 15 Feb 2023 10:11:00 -0500 Subject: [PATCH 5/5] address pr feedback (improve readme) --- instrumentation/README.md | 1 - 1 file changed, 1 deletion(-) diff --git a/instrumentation/README.md b/instrumentation/README.md index 39ae31e9cd..bf32f1888d 100644 --- a/instrumentation/README.md +++ b/instrumentation/README.md @@ -40,7 +40,6 @@ The full path to the auto instrumentation JAR (provided by the installer). Whether to disable service name generation by the .so. If set to "true" (the default), the .so will attempt to generate a service name from the Java command arguments, or provide a hard-coded service name if `service_name` is set. If set to "false", it will not set the service name, leaving it to be generated by the Java auto-instrumentation library. - #### `service_name` (optional) This is an optional override for the service name that would otherwise be generated by the shared object before Java