Skip to content

Feat: filter unneeded mappings in SynchronizeProcess#1267

Merged
christos68k merged 15 commits into
open-telemetry:mainfrom
wehzzz:feat-filter-useless-mappings
Apr 1, 2026
Merged

Feat: filter unneeded mappings in SynchronizeProcess#1267
christos68k merged 15 commits into
open-telemetry:mainfrom
wehzzz:feat-filter-useless-mappings

Conversation

@wehzzz
Copy link
Copy Markdown
Contributor

@wehzzz wehzzz commented Mar 18, 2026 •
edited
Loading

Related issue: #1255

@wehzzz wehzzz changed the title Feat: filter useless mappings in SynchronizeProcess Feat: filter unneeded mappings in SynchronizeProcess Mar 18, 2026
r1viollet
r1viollet reviewed Mar 19, 2026
View reviewed changes
Comment thread process/process.go
@wehzzz wehzzz marked this pull request as ready for review March 19, 2026 08:22
@wehzzz wehzzz requested review from a team as code owners March 19, 2026 08:22
fabled
fabled reviewed Mar 19, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@fabled fabled left a comment

Choose a reason for hiding this comment

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

Thanks so much for working on this. First round of comments added.

Comment thread process/process.go Outdated
Comment thread process/process.go
Comment thread process/process.go Outdated
Comment thread process/types.go
Comment thread processmanager/processinfo.go Outdated
Comment thread processmanager/processinfo.go Outdated
Comment thread processmanager/processinfo.go
@wehzzz
Copy link
Copy Markdown
Contributor Author

wehzzz commented Mar 19, 2026

Thanks for the feedbacks !

@wehzzz wehzzz requested a review from fabled March 19, 2026 14:41
@fabled
Copy link
Copy Markdown
Contributor

fabled commented Mar 21, 2026

@christos68k @florianl Do you have opinion on the process.Mapping to RawMapping struct name change?

I think otherwise this PR is sufficient. In the long term, I'll want to follow up (either be my or someone) with the following additional changes:

  • process API should not have OpenELF, GetMappingFileLastModified or CalculateMappingFileID. This can be abstracted away.
  • processmanager should implement the things it needs the above methods for in a more suitable way. I have also plans to get rid of FileID for ebpf use completely and replace it with something better. The goal is to make FileID informational for reporting things out (not yet decided if it should be used to merge duplicate files in elfCache or not)
  • the OpenMappingFile is the only thing needed, but the signature probably should be changed to include only a minimal subset of the mapping data needed (Vaddr + Length?)
  • need to investigate if it is possible to not use process.RawMapping in most places, but share the processmanager.Mapping instead or other similar data type

Given that the proposed role of RawMapping would be ideally limited between process and processmanager, the name change would be acceptable for me. Thoughts?

christos68k
christos68k reviewed Mar 26, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@christos68k christos68k left a comment

Choose a reason for hiding this comment

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

Left some nits, generally looks OK to me

Comment thread interpreter/types.go Outdated
Comment thread process/process.go Outdated
Comment thread process/process.go Outdated
Comment thread processmanager/processinfo.go Outdated
Comment thread processmanager/processinfo.go Outdated
Comment thread processmanager/processinfo.go Outdated
@christos68k
Copy link
Copy Markdown
Member

christos68k commented Mar 26, 2026

@christos68k @florianl Do you have opinion on the process.Mapping to RawMapping struct name change?

That seems fine to me, if it causes someone to look up the documentation and figure out the semantics around Path handling in the callback it'd have served its purpose.

@github-actions github-actions Bot requested a review from GregMefford March 27, 2026 08:42
@wehzzz wehzzz requested a review from christos68k March 27, 2026 12:27
christos68k
christos68k approved these changes Mar 27, 2026
View reviewed changes
Comment thread process/process.go Outdated
func (sp *systemProcess) getMappingFile(m *Mapping) string {
if m.IsAnonymous() || m.IsVDSO() {
func (sp *systemProcess) getMappingFile(m *RawMapping) string {

Copy link
Copy Markdown
Member

@christos68k christos68k Mar 27, 2026

Choose a reason for hiding this comment

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

Suggested change

@christos68k christos68k mentioned this pull request Mar 27, 2026
Merged
florianl
florianl reviewed Mar 30, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@florianl florianl left a comment

Choose a reason for hiding this comment

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

Do you have opinion on the process.Mapping to RawMapping struct name change?

No strong opinion on this rename from my side.

Comment thread processmanager/processinfo.go
Comment thread processmanager/processinfo.go Outdated
@wehzzz wehzzz requested a review from florianl March 30, 2026 08:11
florianl
florianl approved these changes Mar 30, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@florianl florianl left a comment

Choose a reason for hiding this comment

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

Just a minor nit I didn't spot earlier. Thanks for the work 🙏

Comment thread process/process.go
@wehzzz wehzzz force-pushed the feat-filter-useless-mappings branch from 90d7e30 to f02222c Compare March 30, 2026 14:36
@christos68k
Copy link
Copy Markdown
Member

christos68k commented Mar 31, 2026

@fabled Feel free to merge this (not merging yet to allow you to take a final look)

@christos68k christos68k merged commit aecc98c into open-telemetry:main Apr 1, 2026
62 checks passed
@christos68k
Copy link
Copy Markdown
Member

christos68k commented Apr 1, 2026

I merged this today to clear some of the backlog, if there are more changes let's open another PR.

wehzzz added a commit to wehzzz/opentelemetry-ebpf-profiler that referenced this pull request Apr 2, 2026
@wehzzz @fabled
@florianl @christos68k
Feat: filter unneeded mappings in SynchronizeProcess (open-telemetry#…
05d09d5 
…1267)

Co-authored-by: Timo Teräs <timo.teras@iki.fi>
Co-authored-by: Florian Lehner <florian.lehner@elastic.co>
Co-authored-by: Christos Kalkanis <christos.kalkanis@elastic.co>
wehzzz added a commit to DataDog/opentelemetry-ebpf-profiler that referenced this pull request Apr 3, 2026
@wehzzz @fabled
@florianl @christos68k
Feat: filter unneeded mappings in SynchronizeProcess (open-telemetry#…
3113d73 
…1267) (#62)

Co-authored-by: Timo Teräs <timo.teras@iki.fi>
Co-authored-by: Florian Lehner <florian.lehner@elastic.co>
Co-authored-by: Christos Kalkanis <christos.kalkanis@elastic.co>
@nsavoire nsavoire mentioned this pull request Apr 10, 2026
Open
@fabled fabled mentioned this pull request May 2, 2026
Closed
gnurizen pushed a commit to parca-dev/opentelemetry-ebpf-profiler that referenced this pull request May 4, 2026
@wehzzz @fabled
@florianl @christos68k @gnurizen
Feat: filter unneeded mappings in SynchronizeProcess (open-telemetry#…
e50e9a8 
…1267)

Co-authored-by: Timo Teräs <timo.teras@iki.fi>
Co-authored-by: Florian Lehner <florian.lehner@elastic.co>
Co-authored-by: Christos Kalkanis <christos.kalkanis@elastic.co>
gnurizen pushed a commit to parca-dev/opentelemetry-ebpf-profiler that referenced this pull request May 5, 2026
@wehzzz @fabled
@florianl @christos68k @gnurizen
Feat: filter unneeded mappings in SynchronizeProcess (open-telemetry#…
1c06e47 
…1267)

Co-authored-by: Timo Teräs <timo.teras@iki.fi>
Co-authored-by: Florian Lehner <florian.lehner@elastic.co>
Co-authored-by: Christos Kalkanis <christos.kalkanis@elastic.co>
gnurizen pushed a commit to parca-dev/opentelemetry-ebpf-profiler that referenced this pull request May 12, 2026
@wehzzz @fabled
@florianl @christos68k @gnurizen
Feat: filter unneeded mappings in SynchronizeProcess (open-telemetry#…
4ec687d 
…1267)

Co-authored-by: Timo Teräs <timo.teras@iki.fi>
Co-authored-by: Florian Lehner <florian.lehner@elastic.co>
Co-authored-by: Christos Kalkanis <christos.kalkanis@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@florianl florianl florianl approved these changes

@christos68k christos68k christos68k approved these changes

@fabled fabled Awaiting requested review from fabled

@GregMefford GregMefford Awaiting requested review from GregMefford

+3 more reviewers

@nsavoire nsavoire nsavoire left review comments

@r1viollet r1viollet r1viollet left review comments

@Gandem Gandem Gandem approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@wehzzz @fabled @christos68k @florianl @Gandem @nsavoire @r1viollet