Skip to content

(DynamicSelector) fixups for dynamic resource attributes#2498

Open
damemi wants to merge 2 commits into
open-telemetry:mainfrom
damemi:dynamic-attrs-fixups
Open

(DynamicSelector) fixups for dynamic resource attributes#2498
damemi wants to merge 2 commits into
open-telemetry:mainfrom
damemi:dynamic-attrs-fixups

Conversation

@damemi

@damemi damemi commented Jun 25, 2026

Copy link
Copy Markdown
Member

Summary

Some fixups from #2458 (review) (I was a little quick on the merge):

  • Use deadlock safe SendCtx() instead of Send()
  • Update method comment about leaking state and proper cleanup
  • Cleanup stale PIDs from DynamicFlowAttrs
  • Remove arbitrary resource attributes from flow metrics, which currently only export named attributes from attrs_defs (service name and namespace can still be set). maybe a follow up to add more custom attributes there in the future

Validation

@damemi damemi requested review from grcevski and mariomac June 25, 2026 18:02
@damemi damemi requested a review from a team as a code owner June 25, 2026 18:02
@codecov

codecov Bot commented Jun 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 44.44444% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.05%. Comparing base (5e69ede) to head (d2cb325).
⚠️ Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
pkg/selection/dynamic_flow_attrs.go 45.71% 16 Missing and 3 partials ⚠️
pkg/internal/appolly/appolly.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2498      +/-   ##
==========================================
- Coverage   69.08%   69.05%   -0.04%     
==========================================
  Files         345      345              
  Lines       46728    46746      +18     
==========================================
- Hits        32284    32281       -3     
- Misses      12414    12418       +4     
- Partials     2030     2047      +17     
Flag Coverage Δ
integration-test 51.30% <0.00%> (+0.61%) ⬆️
integration-test-arm 27.09% <0.00%> (-0.07%) ⬇️
integration-test-vm-5.15-lts ?
integration-test-vm-6.18-lts ?
k8s-integration-test 35.84% <0.00%> (+0.15%) ⬆️
oats-test 35.42% <0.00%> (-0.05%) ⬇️
unittests 63.30% <48.48%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@grcevski grcevski left a comment

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.

LGTM! Thanks for fixing the copilot comments from the previous PR. I've launched it again here.

Copilot AI left a comment

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.

Pull request overview

This PR applies follow-up fixes to the DynamicSelector-based flow/metadata decoration pipeline, focusing on safer event delivery, clearer lifecycle expectations for kube-store PID registration, and tightening which dynamic attributes are applied to flow (network/stats) metrics.

Changes:

  • Switch process event publication to deadlock-safe SendCtx() for dynamic selector file-info updates.
  • Remove arbitrary dynamic resource attributes from flow metric decoration (retain service name/namespace only).
  • Add PID cleanup for DynamicFlowAttrs to avoid leaking kube-store process registrations, plus new/updated tests.

Reviewed changes

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

Show a summary per file
File Description
pkg/selection/dynamic_flow_attrs.go Removes flow-level resource attribute decoration and adds PID tracking/cleanup for kube-store process registrations.
pkg/selection/dynamic_flow_attrs_test.go Updates assertions for removed resource attrs and adds coverage for PID unregister behavior.
pkg/selection/dynamic_app_ips.go Clarifies ResolveContainerIPs() side effect (PID registration) and cleanup responsibility.
pkg/internal/appolly/appolly.go Uses deadlock-safe SendCtx(ctx, ...) when emitting process events from dynamic selector updates.
pkg/instrumenter/opts.go Updates documentation comment to reflect flow decoration limited to service name/namespace.

Comment thread pkg/selection/dynamic_flow_attrs.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.

3 participants