Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable UDS settings #233

Merged
merged 3 commits into from
Mar 23, 2023
Merged

Enable UDS settings #233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eee9a3c
Enable UDS settings
r1viollet Mar 23, 2023
dee4815
Minor - remove url parameter from the ddprof input
r1viollet Mar 23, 2023
dedc1e1
Minor add some comments around the url setting flow
r1viollet Mar 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion cmake/Format.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ add_custom_target(format COMMAND ${CMAKE_SOURCE_DIR}/tools/style-check.sh)

add_custom_target(format-apply COMMAND ${CMAKE_SOURCE_DIR}/tools/style-check.sh apply)

add_custom_target(help-generate COMMAND ${CMAKE_SOURCE_DIR}/tools/help_generate.sh -b ${CMAKE_BINARY_DIR})
add_custom_target(help-generate COMMAND ${CMAKE_SOURCE_DIR}/tools/help_generate.sh -b
${CMAKE_BINARY_DIR})
4 changes: 1 addition & 3 deletions include/ddprof_input.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ typedef struct DDProfInput {
char *worker_period;
char *internal_stats;
char *tags;
char *url;
char *socket;
char *preset;
char *switch_user;
Expand Down Expand Up @@ -76,9 +75,8 @@ typedef struct DDProfInput {
XX(DD_API_KEY, api_key, A, 'A', 1, input, NULL, "", exp_input.) \
XX(DD_ENV, environment, E, 'E', 1, input, NULL, "", exp_input.) \
XX(DD_AGENT_HOST, host, H, 'H', 1, input, NULL, "localhost", exp_input.) \
XX(DD_SITE, site, I, 'I', 1, input, NULL, "", exp_input.) \
XX(DD_TRACE_AGENT_PORT, port, P, 'P', 1, input, NULL, "8126", exp_input.) \
XX(DD_TRACE_AGENT_URL, url, U, 'U', 1, input, NULL, "", ) \
XX(DD_TRACE_AGENT_URL, url, U, 'U', 1, input, NULL, "", exp_input.) \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, makes sense to move this 👍

XX(DD_SERVICE, service, S, 'S', 1, input, NULL, "myservice", exp_input.) \
XX(DD_VERSION, service_version, V, 'V', 1, input, NULL, "", exp_input.) \
XX(DD_PROFILING_EXPORT, do_export, X, 'X', 1, input, NULL, "yes", exp_input.) \
Expand Down
6 changes: 3 additions & 3 deletions include/exporter_input.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ typedef struct ExporterInput {
const char *api_key; // Datadog api key
const char *environment; // ex: staging / local / prod
const char *host; // agent host ex:162.184.2.1
const char *site; // not used for now
const char *url; // url (can contain port and schema)
const char *port; // port appended to the host IP (ignored in agentless)
const char
*service; // service to identify the profiles (ex:prof-probe-native)
Expand Down Expand Up @@ -60,7 +60,7 @@ static inline DDRes exporter_input_copy(const ExporterInput *src,
DUP_PARAM(api_key);
DUP_PARAM(environment);
DUP_PARAM(host);
DUP_PARAM(site);
DUP_PARAM(url);
DUP_PARAM(port);
DUP_PARAM(service);
DUP_PARAM(service_version);
Expand All @@ -78,7 +78,7 @@ static inline void exporter_input_free(ExporterInput *exporter_input) {
free((char *)exporter_input->api_key);
free((char *)exporter_input->environment);
free((char *)exporter_input->host);
free((char *)exporter_input->site);
free((char *)exporter_input->url);
free((char *)exporter_input->port);
free((char *)exporter_input->service);
free((char *)exporter_input->service_version);
Expand Down
62 changes: 2 additions & 60 deletions src/ddprof_context_lib.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,6 @@ DDRes ddprof_context_set(DDProfInput *input, DDProfContext *ctx) {
ctx->watchers[nwatchers] = input->watchers[nwatchers];
}
ctx->num_watchers = nwatchers;

DDRES_CHECK_FWD(exporter_input_copy(&input->exp_input, &ctx->exp_input));

// Set defaults
ctx->params.upload_period = 60.0;

Expand Down Expand Up @@ -242,63 +239,6 @@ DDRes ddprof_context_set(DDProfInput *input, DDProfContext *ctx) {
}
}

// URL-based host/port override
if (input->url && *input->url) {
LG_NTC("Processing URL: %s", input->url);
char *delim = strchr(input->url, ':');
char *host = input->url;
char *port = NULL;
if (delim && delim[1] == '/' && delim[2] == '/') {
// A colon was found.
// http://hostname:port -> (hostname, port)
// ftp://hostname:port -> error
// hostname:port -> (hostname, port)
// hostname: -> (hostname, default_port)
// hostname -> (hostname, default_port)

// Drop the schema
*delim = '\0';
if (!strncasecmp(input->url, "http", 4) ||
!strncasecmp(input->url, "https", 5)) {
*delim = ':';
host = delim + 3; // Navigate after schema
}
delim = strchr(host, ':');
}

if (delim) {
// Check to see if there is another colon for the port
// We're going to treat this as the port. This is slightly problematic,
// since an invalid port is going to invalidate the default and then throw
// an error later, but for now let's just do what the user told us even if
// it isn't what they wanted. :)
*delim = '\0';
port = delim + 1;
}

// Modify the input structure to reflect the values from the URL. This
// overwrites an otherwise immutable parameter, which is slightly
// unfortunate, but this way it harmonizes with the downstream movement of
// host/port and the input arg pretty-printer.
if (host) {
free((char *)input->exp_input.host);
free((char *)ctx->exp_input.host);
input->exp_input.host = strdup(host); // For the pretty-printer
ctx->exp_input.host = strdup(host);
}
if (port) {
free((char *)input->exp_input.port);
free((char *)ctx->exp_input.port);
input->exp_input.port = strdup(port); // Merely for the pretty-printer
ctx->exp_input.port = strdup(port);
}

// Revert the delimiter in case we want to print the URL later
if (delim) {
*delim = ':';
}
}

ctx->params.sockfd = -1;
ctx->params.wait_on_socket = false;
if (input->socket && strlen(input->socket) > 0) {
Expand Down Expand Up @@ -332,6 +272,8 @@ DDRes ddprof_context_set(DDProfInput *input, DDProfContext *ctx) {
}
}

DDRES_CHECK_FWD(exporter_input_copy(&input->exp_input, &ctx->exp_input));

ddprof::span watchers{ctx->watchers, static_cast<size_t>(ctx->num_watchers)};
if (std::find_if(watchers.begin(), watchers.end(), [](const auto &watcher) {
return watcher.type < PERF_TYPE_MAX;
Expand Down
1 change: 0 additions & 1 deletion src/ddprof_input.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ const char* help_str[DD_KLEN] = {
" The name of the environment to use in the Datadog UI.\n",
[DD_AGENT_HOST] =
" The hostname of the agent. Port should also be specified.\n",
[DD_SITE] = STR_UNDF,
[DD_TRACE_AGENT_PORT] =
" The communication port for the Datadog agent or backend system.\n",
[DD_TRACE_AGENT_URL] =
Expand Down
75 changes: 61 additions & 14 deletions src/exporter/ddprof_exporter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,22 @@ static const int k_size_api_key = 32;

static char *alloc_url_agent(const char *protocol, const char *host,
const char *port) {
size_t expected_size = snprintf(NULL, 0, "%s%s:%s", protocol, host, port);
char *url = (char *)malloc(expected_size + 1);
if (!url) // Early exit on alloc failure
return NULL;

snprintf(url, expected_size + 1, "%s%s:%s", protocol, host, port);
return url;
if (port) {
size_t expected_size = snprintf(NULL, 0, "%s%s:%s", protocol, host, port);
char *url = (char *)malloc(expected_size + 1);
if (!url) // Early exit on alloc failure
return NULL;

snprintf(url, expected_size + 1, "%s%s:%s", protocol, host, port);
return url;
} else {
size_t expected_size = snprintf(NULL, 0, "%s%s", protocol, host);
char *url = (char *)malloc(expected_size + 1);
if (!url) // Early exit on alloc failure
return NULL;
snprintf(url, expected_size + 1, "%s%s", protocol, host);
return url;
}
}

static DDRes create_pprof_file(ddog_Timespec start,
Expand Down Expand Up @@ -73,6 +82,21 @@ static DDRes write_pprof_file(const ddog_EncodedProfile *encoded_profile,
return {};
}

bool contains_port(const char *url) {
const char *port_ptr = strrchr(url, ':');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making a completely irrelevant comment.

Strictly speaking, I don't know whether strrchr() can be used for this purpose, but I don't think it matters for us in any substantial way. Currently we don't allow users to specify a resource as part of the URI, so : would only be valid to tokenize the port.

if (port_ptr != NULL) {
// Check if the characters after the ':' are digits
for (const char *p = port_ptr + 1; *p != '\0'; p++) {
if (!isdigit(*p)) {
return false;
}
}
return true;
} else {
return false;
}
}

DDRes ddprof_exporter_init(const ExporterInput *exporter_input,
DDProfExporter *exporter) {
memset(exporter, 0, sizeof(DDProfExporter));
Expand All @@ -91,20 +115,43 @@ DDRes ddprof_exporter_init(const ExporterInput *exporter_input,
}

if (exporter->_agent) {
exporter->_url =
alloc_url_agent("http://", exporter_input->host, exporter_input->port);
const char *port_str = exporter_input->port;
Copy link
Collaborator

@nsavoire nsavoire Mar 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be probably be extracted to its own function.
I would simplify the logic by stopping mixing url and port:

  • either a url is given in input, in that case it's expected to be a full url (with protocol and port). In this case host and port are just ignored.
  • either no url is given in input and url is built from 'http', host and port.

That way you can even stop distinguishing between agent and agentless.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good idea, though we would need to check if endpoint is reachable on startup,
Adding this permissive config might be bad for reverting it later.

Copy link
Collaborator

@nsavoire nsavoire Mar 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure to understand, why would this be permissive ?


if (exporter_input->url) {
// uds -> no port
if (!strncasecmp(exporter_input->url, "unix", 4)) {
port_str = nullptr;
}
// already port -> no port
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, agreed

else if (contains_port(exporter_input->url)) {
port_str = nullptr;
}
// check if schema is already available
if (strstr(exporter_input->url, "://") != NULL) {
exporter->_url = alloc_url_agent("", exporter_input->url, port_str);
} else {
// not available, assume http
exporter->_url =
alloc_url_agent("http://", exporter_input->url, port_str);
}
} else {
// no url, use default host and port settings
exporter->_url = alloc_url_agent("http://", exporter_input->host,
exporter_input->port);
}
} else {
// site is the usual option for intake
if (exporter->_input.site) {
// agentless mode
if (exporter->_input.url) {
// warning : should not contain intake.profile. (prepended in
// libdatadog_profiling)
exporter->_url = strdup(exporter_input->site);
exporter->_url = strdup(exporter_input->url);
} else {
LG_WRN(
"[EXPORTER] Agentless - Attempting to use host (%s) instead of empty "
"site",
"url",
exporter_input->host);
exporter->_url = strdup(exporter_input->host);
exporter->_url = alloc_url_agent("http://", exporter_input->host,
exporter_input->port);
}
}
if (!exporter->_url) {
Expand Down
16 changes: 12 additions & 4 deletions test/ddprof_exporter-ut.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ void fill_mock_exporter_input(ExporterInput &exporter_input,
exporter_input.agentless = "yes";
exporter_input.environment = "unit-test";
exporter_input.host = url.first.c_str();
exporter_input.site = "datadog_is_cool.com";
exporter_input.url = "datadog_is_cool.com";
exporter_input.port = url.second.c_str();
exporter_input.service = MYNAME;
exporter_input.service_version = "42";
Expand All @@ -104,20 +104,28 @@ TEST(DDProfExporter, url) {
EXPECT_EQ(strcmp(exporter._url, "datadog_is_cool.com"), 0);
ddprof_exporter_free(&exporter);

// Default to host if site not found
// To be discussed : should we fail here ?
exporter_input.site = nullptr;
exporter_input.url = nullptr;
res = ddprof_exporter_init(&exporter_input, &exporter);
EXPECT_TRUE(IsDDResOK(res));
EXPECT_EQ(strcmp(exporter._url, "25.04.1988.0"), 0);
EXPECT_EQ(strcmp(exporter._url, "http://25.04.1988.0:1234"), 0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering that the previous test uses a hostname without an explicit protocol, I think it's fine to convert this to a protocol-bearing statement.

ddprof_exporter_free(&exporter);

// If no API key --> expect host
fill_mock_exporter_input(exporter_input, url, false);
exporter_input.url = nullptr;
res = ddprof_exporter_init(&exporter_input, &exporter);
EXPECT_TRUE(IsDDResOK(res));
EXPECT_EQ(strcmp(exporter._url, "http://25.04.1988.0:1234"), 0);
ddprof_exporter_free(&exporter);

// UDS --> expect UDS
fill_mock_exporter_input(exporter_input, url, false);
exporter_input.url = "unix://some/uds/socket.sock";
res = ddprof_exporter_init(&exporter_input, &exporter);
EXPECT_TRUE(IsDDResOK(res));
EXPECT_EQ(strcmp(exporter._url, "unix://some/uds/socket.sock"), 0);
ddprof_exporter_free(&exporter);
}

TEST(DDProfExporter, simple) {
Expand Down