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

Jump to bottom

Implementation of support for custom resource attributes #54

Closed
wants to merge 2 commits into from
Closed

Implementation of support for custom resource attributes #54

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
158c55f
Add feature custom resource attributes
May 28, 2024
c39f334
Add tests for feature custom resource attributes
May 28, 2024
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
16 changes: 15 additions & 1 deletion src/batch_exporter.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@
#include "trace_context.hpp"
#include "trace_service_client.hpp"

struct ResourceAttr {
ngx_str_t name;
ngx_http_complex_value_t value;
};

class BatchExporter {
public:
typedef TraceServiceClient::Request Request;
Expand Down Expand Up @@ -112,7 +117,7 @@ class BatchExporter {
};

BatchExporter(StrView target,
size_t batchSize, size_t batchCount, StrView serviceName) :
size_t batchSize, size_t batchCount, StrView serviceName, ngx_array_t customResourceAttrs) :
Copy link
Contributor

Choose a reason for hiding this comment

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

For most part, we try to avoid "ngx_*" types in this file and use C++ types. Looks like, for attributes we can use const std::map<StrView, StrView>& attrs.

batchSize(batchSize), client(std::string(target))
{
free.reserve(batchCount);
Expand All @@ -124,6 +129,15 @@ class BatchExporter {
attr->set_key("service.name");
Copy link
Contributor

Choose a reason for hiding this comment

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

If we have explicit otel_service_name directive, it shouldn't be overridden by resource attributes block, which is currently the case. Although, it should be possible to define "service.name" via block, if there is no explicit otel_service_name. Makes sense to remove serviceName argument and keep that logic outside.

attr->mutable_value()->set_string_value(std::string(serviceName));

auto attrs = (ResourceAttr*)customResourceAttrs.elts;
for (ngx_uint_t i = 0; i < customResourceAttrs.nelts; i++) {
attr = resourceSpans->mutable_resource()->add_attributes();
StrView value = StrView((char*)attrs[i].value.value.data, attrs[i].value.value.len);
Copy link
Contributor

Choose a reason for hiding this comment

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

That's not how complex values work. We can't really use complex value here, as those require client request to be evaluated.

StrView name = StrView((char*)attrs[i].name.data, attrs[i].name.len);
attr->set_key(std::string(name));
attr->mutable_value()->set_string_value(std::string(value));
}

auto scopeSpans = resourceSpans->add_scope_spans();
scopeSpans->mutable_scope()->set_name("nginx");
scopeSpans->mutable_scope()->set_version(NGINX_VERSION);
Expand Down
65 changes: 64 additions & 1 deletion src/http_module.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ struct MainConf {
size_t batchCount;

ngx_str_t serviceName;
ngx_array_t resourceAttrs;
};

struct SpanAttr {
Expand All @@ -39,6 +40,7 @@ struct LocationConf {

char* setExporter(ngx_conf_t* cf, ngx_command_t* cmd, void* conf);
char* addSpanAttr(ngx_conf_t* cf, ngx_command_t* cmd, void* conf);
char* addResourceAttrs(ngx_conf_t* cf, ngx_command_t* cmd, void* conf);

namespace Propagation {

Expand Down Expand Up @@ -67,6 +69,12 @@ ngx_command_t gCommands[] = {
ngx_conf_set_str_slot,
NGX_HTTP_MAIN_CONF_OFFSET,
offsetof(MainConf, serviceName) },

{ ngx_string("otel_resource_attr"),
Copy link
Contributor

Choose a reason for hiding this comment

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

The idea was to call the block "otel_resource" and, perhaps, add some other resource related options to it later on (or even support multiple resources).

"otel_resource_attr" name makes more sense for single attribute directive, like otel_resource_attr name value;. It's a reasonable approach, but then it shouldn't be a block.

NGX_HTTP_MAIN_CONF|NGX_CONF_BLOCK|NGX_CONF_NOARGS,
addResourceAttrs,
NGX_HTTP_MAIN_CONF_OFFSET,
offsetof(MainConf, resourceAttrs) },

{ ngx_string("otel_trace"),
NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
Expand Down Expand Up @@ -455,6 +463,7 @@ void addCustomAttrs(BatchExporter::Span& span, ngx_http_request_t* r)
}
}


Copy link
Contributor

Choose a reason for hiding this comment

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

Accidental edit?

ngx_int_t onRequestEnd(ngx_http_request_t* r)
{
auto ctx = getOtelCtx(r);
Expand Down Expand Up @@ -540,7 +549,9 @@ ngx_int_t initWorkerProcess(ngx_cycle_t* cycle)
toStrView(mcf->endpoint),
mcf->batchSize,
mcf->batchCount,
toStrView(mcf->serviceName)));
toStrView(mcf->serviceName),
mcf->resourceAttrs
));
} catch (const std::exception& e) {
ngx_log_error(NGX_LOG_CRIT, cycle->log, 0,
"OTel worker init error: %s", e.what());
Expand Down Expand Up @@ -705,6 +716,58 @@ char* addSpanAttr(ngx_conf_t* cf, ngx_command_t* cmd, void* conf)
return NGX_CONF_OK;
}

char* addResourceAttr(ngx_conf_t* cf, ngx_command_t* cmd, void* conf) {
auto mcf = (MainConf*)conf;

if (mcf->resourceAttrs.elts == NULL && ngx_array_init(&mcf->resourceAttrs,
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a similar code below. Is that intentional?

cf->pool, 4, sizeof(ResourceAttr)) != NGX_OK) {
return (char*)NGX_CONF_ERROR;
}

auto attr = (ResourceAttr*)ngx_array_push(&mcf->resourceAttrs);
if (attr == NULL) {
return (char*)NGX_CONF_ERROR;
}

auto args = (ngx_str_t*)cf->args->elts;

attr->name = args[0];

ngx_http_compile_complex_value_t ccv = { cf, &args[1], &attr->value };
Copy link
Contributor

Choose a reason for hiding this comment

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

What if there is only one arg?

if (ngx_http_compile_complex_value(&ccv) != NGX_OK) {
return (char*)NGX_CONF_ERROR;
}

return NGX_CONF_OK;
}

char* addResourceAttrs(ngx_conf_t* cf, ngx_command_t* cmd, void* conf)
{
auto mcf = (MainConf*)conf;

if (mcf->resourceAttrs.elts == NULL && ngx_array_init(&mcf->resourceAttrs,
cf->pool, 4, sizeof(ResourceAttr)) != NGX_OK) {
return (char*)NGX_CONF_ERROR;
}

auto cfCopy = *cf;

cfCopy.handler = addResourceAttr;
cfCopy.handler_conf = mcf;
auto rv = ngx_conf_parse(&cfCopy, NULL);
if (rv != NGX_CONF_OK) {
return rv;
}

if (mcf->endpoint.len == 0) {
ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
"\"otel_exporter\" requires \"endpoint\"");
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this related?

return (char*)NGX_CONF_ERROR;
}

return NGX_CONF_OK;
}

template <class Id>
ngx_int_t hexIdVar(ngx_http_request_t* r, ngx_http_variable_value_t* v,
uintptr_t data)
Expand Down
9 changes: 8 additions & 1 deletion tests/h2_otel.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ http {
batch_count 1;
}

otel_resource_attr {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given, these attributes are not HTTP version related, I don't think we need to add them into h2/h3 tests.

service.namespace test;
}

otel_service_name test_server;
otel_trace on;

Expand Down Expand Up @@ -159,7 +163,7 @@ foreach my $name ('localhost') {
or die "Can't create certificate for $name: $!\n";
}

$t->try_run('no OTel module')->plan(69);
$t->try_run('no OTel module')->plan(70);

###############################################################################

Expand Down Expand Up @@ -211,6 +215,9 @@ is(scalar keys %{$$batch1{scope_spans}}, 5, 'batch1 size - trace on');
is(get_attr("service.name", "string_value",
$$batch0{resource}),
'test_server', 'service.name - trace on');
is(get_attr("service.namespace", "string_value",
$$batch0{resource}),
'test', 'service.namespace - trace on');
is($$spans{span0}{name}, '"default_location"', 'span.name - trace on');

#validate http metrics
Expand Down
9 changes: 8 additions & 1 deletion tests/h3_otel.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ http {
batch_count 2;
}

otel_resource_attr {
service.namespace test;
}

otel_service_name test_server;
otel_trace on;

Expand Down Expand Up @@ -159,7 +163,7 @@ foreach my $name ('localhost') {
or die "Can't create certificate for $name: $!\n";
}

$t->try_run('no OTel module')->plan(56);
$t->try_run('no OTel module')->plan(57);

###############################################################################

Expand Down Expand Up @@ -211,6 +215,9 @@ is(scalar keys %{$$batch1{scope_spans}}, 5, 'batch1 size - trace on');
is(get_attr("service.name", "string_value",
$$batch0{resource}),
'test_server', 'service.name - trace on');
is(get_attr("service.namespace", "string_value",
$$batch0{resource}),
'test', 'service.namespace - trace on');
is($$spans{span0}{name}, '"default_location"', 'span.name - trace on');

#validate metrics
Expand Down
9 changes: 8 additions & 1 deletion tests/otel.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ http {
batch_count 2;
}

otel_resource_attr {
Copy link
Contributor

Choose a reason for hiding this comment

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

Note indent.

service.namespace test;
}

otel_service_name test_server;
otel_trace on;

Expand Down Expand Up @@ -156,7 +160,7 @@ foreach my $name ('localhost') {
or die "Can't create certificate for $name: $!\n";
}

$t->try_run('no OTel module')->plan(69);
$t->try_run('no OTel module')->plan(70);

###############################################################################

Expand Down Expand Up @@ -203,6 +207,9 @@ is(scalar keys %{$$batch1{scope_spans}}, 5, 'batch1 size - trace on');
#validate general attributes
is(get_attr("service.name", "string_value",
$$batch0{resource}), 'test_server', 'service.name - trace on');
is(get_attr("service.namespace", "string_value",
$$batch0{resource}),
'test', 'service.namespace - trace on');
is($$spans{span0}{name}, '"default_location"', 'span.name - trace on');

#validate http metrics
Expand Down
9 changes: 8 additions & 1 deletion tests/otel_collector.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ http {
batch_count 2;
}

otel_resource_attr {
service.namespace test;
}

otel_service_name test_server;
otel_trace on;

Expand Down Expand Up @@ -162,7 +166,7 @@ open STDERR, ">&", \*OLDERR;
$t->waitforsocket('127.0.0.1:' . port(4317)) or
die 'No otel collector open socket';

$t->try_run('no OTel module')->plan(69);
$t->try_run('no OTel module')->plan(70);

###############################################################################

Expand Down Expand Up @@ -209,6 +213,9 @@ is(scalar @{${JSON::PP::decode_json($batches[1])}{"resourceSpans"}[0]
is(get_attr("service.name", "stringValue",
$$batch_json{resourceSpans}[0]{resource}),
'test_server', 'service.name - trace on');
is(get_attr("service.namespace", "stringValue",
$$batch_json{resourceSpans}[0]{resource}),
'test', 'service.namespace - trace on');
is($$spans[0]{name}, 'default_location', 'span.name - trace on');

#validate http metrics
Expand Down