Enable UDS settings #233
Enable UDS settings #233
Conversation
Ensure UDS settings are allowed by the exporter
Parameter is now written directly to the exporter.
| @@ -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; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Not sure to understand, why would this be permissive ?
There was a problem hiding this comment.
I think this looks good. At some point we should probably change the input stuff to using std::string or something instead of raw C strings, but I don't think this is really the time or the place for that. :)
Good stuff! Thank you! UDS support may not improve performance substantially, since we use async, but it's still a configuration users will want!
| 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.) \ |
There was a problem hiding this comment.
Ah, makes sense to move this 👍
| if (!strncasecmp(exporter_input->url, "unix", 4)) { | ||
| port_str = nullptr; | ||
| } | ||
| // already port -> no port |
| 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); |
There was a problem hiding this comment.
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.
| @@ -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, ':'); | |||
There was a problem hiding this comment.
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.
|
Merging as is for now, I agree there should be a follow up on this. |
* Enable UDS settings Ensure UDS settings are allowed by the exporter
What does this PR do?
Ensure UDS settings are allowed by the exporter
Motivation
Allow usage of UDS.
#232
Testing
Testing is ongoing