-
Notifications
You must be signed in to change notification settings - Fork 174
Add opentelemetry_resource_attr #566
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.
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
base: main
Are you sure you want to change the base?
Add opentelemetry_resource_attr #566
Conversation
Signed-off-by: Yuri Oliveira <[email protected]>
Signed-off-by: Yuri Oliveira <[email protected]>
@open-telemetry/cpp-contrib-approvers, would it be possible to re-run only the failed jobs? |
Signed-off-by: Yuri Oliveira <[email protected]>
@seemk @tobiasstadler - Can someone review this. thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for custom resource attributes in the OpenTelemetry nginx instrumentation by introducing the opentelemetry_resource_attr
configuration directive. This enhancement allows users to add custom resource attributes like service version and deployment environment to their traces.
- Adds
opentelemetry_resource_attr
directive to set custom resource attributes - Updates the tracer provider initialization to use custom resource attributes instead of hardcoded service.name
- Preserves backward compatibility by defaulting service.name if not provided via resource attributes
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
instrumentation/nginx/src/agent_config.h |
Adds resourceAttributes map to store custom resource attributes |
instrumentation/nginx/src/otel_ngx_module.cpp |
Implements the new directive handler and updates tracer provider initialization logic |
instrumentation/nginx/test/conf/nginx.conf |
Adds test configuration with sample resource attributes |
instrumentation/nginx/test/instrumentation/test/instrumentation_test.exs |
Adds test assertions for the new resource attributes |
instrumentation/nginx/README.md |
Documents the new opentelemetry_resource_attr directive |
@@ -1051,6 +1051,21 @@ char* OtelNgxSetTracesSamplerRatio(ngx_conf_t* cf, ngx_command_t*, void*) { | |||
return NGX_CONF_OK; | |||
} | |||
|
|||
char* OtelNgxSetResourceAttr(ngx_conf_t* cf, ngx_command_t*, void*) { | |||
OtelMainConf* otelMainConf = GetOtelMainConf(cf); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function should validate that exactly 2 arguments are provided (key and value). Currently, if fewer than 2 arguments are passed, accessing values[2] could cause undefined behavior.
if (cf->args->nelts < 3) { | |
ngx_log_error(NGX_LOG_ERR, cf->log, 0, "opentelemetry: invalid number of arguments for resource attribute. Expected 2 arguments (key and value)."); | |
return (char*)NGX_CONF_ERROR; | |
} |
Copilot uses AI. Check for mistakes.
std::string strKey((const char*)key->data, key->len); | ||
std::string strValue((const char*)value->data, value->len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key and value strings are constructed without null-termination validation. If key->data or value->data are not null-terminated within the specified length, this could lead to buffer overruns when used elsewhere.
std::string strKey((const char*)key->data, key->len); | |
std::string strValue((const char*)value->data, value->len); | |
std::string strKey; | |
std::string strValue; | |
{ | |
std::vector<char> keyBuffer(key->data, key->data + key->len); | |
keyBuffer.push_back('\0'); // Ensure null-termination | |
strKey = std::string(keyBuffer.data()); | |
} | |
{ | |
std::vector<char> valueBuffer(value->data, value->data + value->len); | |
valueBuffer.push_back('\0'); // Ensure null-termination | |
strValue = std::string(valueBuffer.data()); | |
} |
Copilot uses AI. Check for mistakes.
@@ -40,7 +40,9 @@ load_module /path/to/otel_ngx_module.so; | |||
|
|||
http { | |||
opentelemetry_service_name "nginx-proxy"; | |||
opentelemetry_otlp_traces_endpoint "http://collector:4318/v1/traces" | |||
opentelemetry_otlp_traces_endpoint "http://collector:4318/v1/traces"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be an unrelated formatting fix (adding missing semicolon) that should be mentioned in the PR description or handled in a separate commit.
Copilot uses AI. Check for mistakes.
Signed-off-by: Yuri Oliveira <[email protected]>
It's also related to the #404. |
@lalitb, I've implemented the changes suggested by Copilot, and I believe the PR is now ready to go. |
Add opentelemetry_resource_attr
Resolves: nginx: Configure custom resource attributes #232