Skip to content

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

yuriolisa
Copy link

Signed-off-by: Yuri Oliveira <[email protected]>
@yuriolisa yuriolisa requested review from seemk, tobiasstadler and a team as code owners July 28, 2025 13:32
@yuriolisa yuriolisa changed the title Add opentelemetry_resource_attr [WIP] Add opentelemetry_resource_attr Jul 28, 2025
Signed-off-by: Yuri Oliveira <[email protected]>
@yuriolisa yuriolisa changed the title [WIP] Add opentelemetry_resource_attr Add opentelemetry_resource_attr Jul 28, 2025
@yuriolisa
Copy link
Author

@open-telemetry/cpp-contrib-approvers, would it be possible to re-run only the failed jobs?

Signed-off-by: Yuri Oliveira <[email protected]>
@lalitb lalitb requested a review from Copilot July 28, 2025 16:54
@lalitb
Copy link
Member

lalitb commented Jul 28, 2025

@seemk @tobiasstadler - Can someone review this. thanks.

Copy link

@Copilot Copilot AI left a 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);

Copy link
Preview

Copilot AI Jul 28, 2025

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.

Suggested change
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.

Comment on lines +1061 to +1062
std::string strKey((const char*)key->data, key->len);
std::string strValue((const char*)value->data, value->len);
Copy link
Preview

Copilot AI Jul 28, 2025

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.

Suggested change
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";
Copy link
Preview

Copilot AI Jul 28, 2025

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]>
@yuriolisa
Copy link
Author

It's also related to the #404.

@yuriolisa
Copy link
Author

@lalitb, I've implemented the changes suggested by Copilot, and I believe the PR is now ready to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nginx: Configure custom resource attributes
2 participants