Implementation of support for custom resource attributes #54
Conversation
puhre
force-pushed
the
custom-resource-attributes
branch
from
90eba3d
to
c39f334
Compare
|
This is just what I need. Any progress on having this reviewed? |
| @@ -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) : | |||
There was a problem hiding this comment.
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.
| @@ -124,6 +129,15 @@ class BatchExporter { | |||
| attr->set_key("service.name"); | |||
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
That's not how complex values work. We can't really use complex value here, as those require client request to be evaluated.
| @@ -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"), | |||
There was a problem hiding this comment.
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.
|
|
||
| if (mcf->endpoint.len == 0) { | ||
| ngx_conf_log_error(NGX_LOG_EMERG, cf, 0, | ||
| "\"otel_exporter\" requires \"endpoint\""); |
|
|
||
| attr->name = args[0]; | ||
|
|
||
| ngx_http_compile_complex_value_t ccv = { cf, &args[1], &attr->value }; |
There was a problem hiding this comment.
What if there is only one arg?
| 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, |
There was a problem hiding this comment.
There is a similar code below. Is that intentional?
| @@ -455,6 +463,7 @@ void addCustomAttrs(BatchExporter::Span& span, ngx_http_request_t* r) | |||
| } | |||
| } | |||
|
|
|||
|
|
|||
| @@ -47,6 +47,10 @@ http { | |||
| batch_count 1; | |||
| } | |||
|
|
|||
| otel_resource_attr { | |||
There was a problem hiding this comment.
Given, these attributes are not HTTP version related, I don't think we need to add them into h2/h3 tests.
| @@ -46,6 +46,10 @@ http { | |||
| batch_count 2; | |||
| } | |||
|
|
|||
| otel_resource_attr { | |||
|
Hey, what's the status of this PR? Although I don't have much experience in |
I have moved to another solution which does not involve nginx so this is not a priority for me anymore. If you want to go ahead and continue this PR by resolving the comments you are more than welcome. I will probably not get around to it for a while. |
|
The functionality will be added via #74. |
Proposed changes
This pull requests adds support for optional resources attributes. The resource attributes are configured, similar to what was suggested in the Issue #32, using a block configuration, e.g.
fix Custom Resource Attributes
Checklist
Before creating a PR, run through this checklist and mark each as complete.
CONTRIBUTINGdocument