fix: reduce log noise and filename duplication

[areebahmeddd]

Signed-off-by: Areeb Ahmed areebshariff@acm.org

Summary of Changes

Changes Golang interpreter loader error handling to reduce log noise and eliminate filename duplication:

• Change log level from error to debug for "failed to create point resolver" messages in execinfomanager
• Remove executable path from Go interpreter error message to prevent duplication (path is already logged by execinfomanager)
• Add function to map standard go errors (C.SYMBLIB_ERR_IOFILENOTFOUND, C.SYMBLIB_ERR_OBJFILE, C.SYMBLIB_ERR_DWARF)

Related Issue(s)

Closes #466

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: areebahmeddd / name: Areeb Ahmed (b0042ad, 071c022, 0841064, 483e289, fbb8957, 604f8ae, 2cf0a5d)

[florianl]

While changing the visibility of the log line from Error to Debug, reduces the logged output, if the OTel eBPF profiler is not running in verbose mode, it does not resolve the underlying issue, that causes the error log line.

The log lines in #466 indicate a race condition somewhere between Go and Rust (?). As a result, symbolization will fail for this part. I would prefer, if we could fix the race condition and guarantee less symbolization issues, rather changing the visibility for issue.

[rockdaboot]

The log lines in #466 indicate a race condition somewhere between Go and Rust (?). As a result, symbolization will fail for this part. I would prefer, if we could fix the race condition and guarantee less symbolization issues, rather changing the visibility for issue.

@florianl are you sure this isn't just an expected TOCTOU? If it is, using debug level seems fine to me. Otherwise, I agree with you that the issue should be fixed and the log should stay at error level.

@areebahmeddd You are addressing two different issues in your PR, a) simplifying the error message and b) changing the log level / user-visibility.
Since b) is debatable and needs further considerations, I suggest to restore the log level back to error in order to proceed.

[areebahmeddd]

@rockdaboot updated the pr! ✅

@florianl umm not very familiar with the codebase so from my understanding status code 2 indicates symblib err file not found, so a straightforward fix would be adding a retry mechanism that waits for extracted files to become accessible before passing them to symblib?

[florianl]

are you sure this isn't just an expected TOCTOU?

If you compare the current Rust based approach with #456, you will notice that this isn't just an expected TOCTOU - otherwise you would see it with #456 as well.

[..] a straightforward fix would be adding a retry mechanism that waits for extracted files to become accessible before passing them to symblib?

#455 in combination with #456 should resolve #466.

[rockdaboot]

#455 in combination with #456 should resolve #466.

I see, thanks for pointing this out.
Since we don't know when that will be merged, it won't hurt to improve the log message right now. Not a strong "must have", though. It's just a little detail. So I will approve, but @areebahmeddd feel free to close this PR in favor of #456.

[areebahmeddd]

Anything works! : )

[fabled]

The change on the error message looks good. But the status should be inspected and a equivalent Golang error should be instead embedded if possible.

[fabled]

I think the status should be mapped to standard Golang errors where possible. E.g. the -2 indicating file not found, should return an error with os.ErrNotExist embedded in it. This allows the main code path to omit the error logging: its already testing for this specific error code and its not logged.

The root case for this error is indeed race condition. It happens when the earlier code path has opened the ELF file with pfelf.Open successfully, but then during processing of stack deltas or other data the process exits. This causes the above function fail as the mappings file does not exist anymore.

In other interpreters this is already handled correctly, and it does not manifest if this is converted to Go code, because then there is no more attempt to re-open the ELF file with a filename - the cached pfelf.File or the underlying ReaderAt will get used.

[areebahmeddd]

Cool! I've created a function to map standard go errors! Hope it's matches your requirement 🙂

[fabled]

#455 in combination with #456 should resolve #466.

I think #456 will take quite a bit of time. Using the Golang built in library will likely explode memory usage. We really need to implement the code in our own code using the mmapped file. The same way the Rust code does it.

I think it makes sense to merge this first. Ideally with the mapping of status to os.ErrNotExist when appropriate.

[fabled]

LGTM. Thanks!