Skip to content

fix: target_info skipped when resource attributes have empty values#6774

Merged
carles-grafana merged 1 commit intografana:mainfrom
carles-grafana:fix/target-info-empty-resource-attr
Mar 25, 2026
Merged

fix: target_info skipped when resource attributes have empty values#6774
carles-grafana merged 1 commit intografana:mainfrom
carles-grafana:fix/target-info-empty-resource-attr

Conversation

@carles-grafana
Copy link
Copy Markdown
Contributor

The Prometheus labels.Builder deletes labels with empty values, but resourceAttributesCount was computed before building. This mismatch caused the guard check to fail, silently dropping target_info for any service with empty resource attributes (e.g. host.id="" from the OTel NodeJS SDK).

Replace the count comparison with direct checks for job/instance.

What this PR does:

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Copilot AI review requested due to automatic review settings March 25, 2026 14:10
Copy link
Copy Markdown
Contributor

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 fixes a regression where traces_target_info could be skipped when resource attributes include empty-string values, due to a mismatch between raw resource attribute counts and the final label set produced by the Prometheus label builder.

Changes:

  • Replace the target_info emission guard from a label-count comparison to explicit job/instance presence checks.
  • Add a regression test that exercises empty-valued resource attributes while still expecting traces_target_info to be emitted.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
modules/generator/processor/spanmetrics/spanmetrics.go Updates the target_info guard logic to avoid relying on label counts that can be affected by empty-valued labels being dropped.
modules/generator/processor/spanmetrics/spanmetrics_test.go Adds a regression test for empty-valued resource attributes to ensure traces_target_info is still generated.

Comment thread modules/generator/processor/spanmetrics/spanmetrics.go Outdated
Copy link
Copy Markdown
Contributor

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment thread modules/generator/processor/spanmetrics/spanmetrics.go Outdated
Comment thread modules/generator/processor/spanmetrics/spanmetrics_test.go Outdated
Copy link
Copy Markdown
Contributor

@ie-pham ie-pham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice find lgtm

if jobName != "" {
identifyingLabels++
}
if instanceID != "" && p.Cfg.EnableInstanceLabel {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This two lines are duplicated above

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

// resourceAttributesCount, because the Prometheus label builder drops empty-valued
// labels (Set("x","") calls Del("x")), which would cause a count mismatch.
identifyingLabels := 0
if jobName != "" {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is already calculated above, we can just coung the indentifiying labels in the already existing conditions

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

// only register target info if at least (job or instance) AND one other attribute are present
// TODO - We can move this check to the top
if resourceAttributesCount > 0 && targetInfoRegistryLabelValues.Len() > resourceAttributesCount {
// Only register target_info if it has at least (job or instance) AND one other
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is referencig the deleted variable resourceAttributesCount

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@carles-grafana carles-grafana force-pushed the fix/target-info-empty-resource-attr branch from 3f65492 to 5c2d82d Compare March 25, 2026 16:15
The Prometheus labels.Builder deletes labels with empty values, but
resourceAttributesCount was computed before building. This mismatch
caused the guard check to fail, silently dropping target_info for any
service with empty resource attributes (e.g. host.id="" from the
OTel NodeJS SDK).

Count identifying labels (job/instance) and compare against the built
label set length to ensure at least one real resource attribute exists.
@carles-grafana carles-grafana force-pushed the fix/target-info-empty-resource-attr branch from 5c2d82d to 3b61bf3 Compare March 25, 2026 16:24
Copilot AI review requested due to automatic review settings March 25, 2026 16:24
Copy link
Copy Markdown
Contributor

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment on lines 190 to 200
// add job label only if job is not blank and target_info is enabled
identifyingLabels := 0
if jobName != "" && p.Cfg.EnableTargetInfo {
builder.Add(gen.DimJob, jobName)
identifyingLabels++
}
// add instance label only if instance is not blank and enabled and target_info is enabled
// add instance label only if instance is not blank and enabled and target_info is enabled
if instanceID != "" && p.Cfg.EnableTargetInfo && p.Cfg.EnableInstanceLabel {
builder.Add(gen.DimInstance, instanceID)
identifyingLabels++
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

identifyingLabels is incremented when adding job/instance to the span metrics label builder, but it’s later used to decide whether to emit target_info. This couples two separate builders and could become incorrect if the conditions for adding job/instance diverge in the future. Consider computing this count (or two booleans for job/instance presence) alongside the targetInfoBuilder.Add(...) calls inside the EnableTargetInfo block, and use that for the guard.

Copilot uses AI. Check for mistakes.
@carles-grafana carles-grafana merged commit 5c424fb into grafana:main Mar 25, 2026
48 of 49 checks passed
carles-grafana pushed a commit to carles-grafana/tempo that referenced this pull request Mar 27, 2026
Add 3 entries that were missing from the unreleased changelog:
- [BUGFIX] grafana#6774: target_info skipped when resource attributes have empty values
- [BUGFIX] grafana#6653: drain old series on metric replacement to prevent limiter leak
- [ENHANCEMENT] grafana#6371: make trace_too_large log line an insight
carles-grafana added a commit that referenced this pull request Mar 27, 2026
Add 3 entries that were missing from the unreleased changelog:
- [BUGFIX] #6774: target_info skipped when resource attributes have empty values
- [BUGFIX] #6653: drain old series on metric replacement to prevent limiter leak
- [ENHANCEMENT] #6371: make trace_too_large log line an insight

Co-authored-by: Carles Grafana <carles@grafana.com>
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.

4 participants