Skip to content

Fix otel_span_attr merging across config levels (#107).#108

Open
danielkatz wants to merge 1 commit into
nginx:mainfrom
danielkatz:fix/otel-span-attr-merge
Open

Fix otel_span_attr merging across config levels (#107).#108
danielkatz wants to merge 1 commit into
nginx:mainfrom
danielkatz:fix/otel-span-attr-merge

Conversation

@danielkatz
Copy link
Copy Markdown

Previously, when otel_span_attr directives were declared at both http and server levels, only the child level attributes were reported. This fix merges attributes from parent levels with child levels, ensuring all configured attributes are included in spans.

Proposed changes

This PR fixes issue #107 where otel_span_attr directives declared at different configuration levels (http/server/location) were not merged. Previously, only the most specific level's attributes were reported, with parent level attributes being silently discarded.

Root Cause:
The mergeLocationConf function in src/http_module.cpp only inherited parent attributes when the child had none. If the child had any otel_span_attr directives, the parent's attributes were completely replaced rather than merged.

Solution:

  • Modified mergeLocationConf to merge parent and child span attributes into a single array
  • Parent attributes are added first, followed by child attributes (allowing child values to override parent values for the same key at the collector level)
  • Added proper error handling for memory allocation failures
  • Maintains backward compatibility - existing configurations continue to work unchanged

Changes:

  • src/http_module.cpp: Enhanced merge logic to concatenate parent and child span attributes
  • tests/test_otel.py:
    • Added get_all_attrs() helper function to retrieve all values for duplicate attribute keys
    • Added test_span_attr_merge() to verify http + server level merging
    • Added test_span_attr_merge_with_location() to verify all three levels merge correctly
    • Updated NGINX_CONFIG template to support http/server level span attributes

Testing:
All 26 tests pass, including the 2 new tests that specifically verify the merge behavior.

Fixes #107

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING document
  • If applicable, I have added tests that prove my fix is effective or that my feature works
  • If applicable, I have checked that any relevant tests pass after adding my changes

Previously, when otel_span_attr directives were declared at both
http and server levels, only the child level attributes were
reported. This fix merges attributes from parent levels with child
levels, ensuring all configured attributes are included in spans.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

otel_span_attr from different levels are not merged

2 participants