Skip to content

Conversation

@yuriolisa
Copy link

Signed-off-by: Yuri Oliveira <[email protected]>
@yuriolisa yuriolisa requested review from a team, seemk and tobiasstadler 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 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


char* OtelNgxSetResourceAttr(ngx_conf_t* cf, ngx_command_t*, void*) {
OtelMainConf* otelMainConf = GetOtelMainConf(cf);

Copy link

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

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

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.

@ThomsonTan
Copy link
Contributor

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

@yuriolisa could you please address the CI failure?

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

3 participants