Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

process: Add build_id to executable #1329

Merged
merged 15 commits into from
Sep 11, 2024

Conversation

florianl
Copy link
Contributor

@florianl florianl commented Aug 9, 2024

This is a follow up from a discussion in #1188 (comment).

UPDATE:

Changes

In the OTel Profiling use case, symbolization is important to resolve numeric addresses into human readable (function-) names. For this use case it is important to uniquely identify executables. There are existing and well established identifiers for executables, like GNU Build ID and Go Build ID.
This proposed change adds these identifiers to the process.executable attribute.

Merge requirement checklist

For visibility: @open-telemetry/profiling-maintainers

@florianl florianl requested review from a team August 9, 2024 08:42
model/registry/process.yaml Show resolved Hide resolved
model/registry/process.yaml Show resolved Hide resolved
model/registry/process.yaml Show resolved Hide resolved
@jsuereth
Copy link
Contributor

If no further comments/updates happen before monday, I'd like to talk about this in the general semconv meeting.

From what I can tell - you may need something stronger then semantic conventions here for profiling to work, but I want to confirm. Would you be able to attend the meeting Monday at 7am PDT to discuss in person?

If not, I'll give the gist of the discusison here:

  • Semantic conventions are optional meaning, while instrumentaiton from the OpenTelemetry ecosystem has to abide by it, the general community does not.
  • If attaching symbols to profiles is something that must work for Profiles to be usable, generally, you may need a stronger convention for recording this, e.g. a direct Proto field in OTLP.

I think the answer to this question would resolve my concerns: Will profiles be useful if we can't find symbol tables for executables?

If the answer is "not really": we can use semconv, but we may want to full up specify the attributes and force semconv to have them defined like these attributes

@jsuereth
Copy link
Contributor

If the answer is "not really": we can use semconv, but we may want to full up specify the attributes and force semconv to have them defined like these attributes

Discussed in the meeting today - and I think the answer is no really, we need flexibility here, so these belong in semconv.

florianl and others added 3 commits August 22, 2024 16:39
@florianl
Copy link
Contributor Author

Here is an example of how the build ID can be used with public available information, to get symbol/debug information:

$ DEBUGINFOD_URLS="https://debuginfod.elfutils.org/" debuginfod-find -vv debuginfo c89b11207f6479603b0d49bf291c092c2b719293
debuginfod_find_debuginfo c89b11207f6479603b0d49bf291c092c2b719293
server urls "https://debuginfod.elfutils.org/"
checking build-id
checking cache dir /home/user/.cache/debuginfod_client
using timeout 90
init server 0 https://debuginfod.elfutils.org/buildid
url 0 https://debuginfod.elfutils.org/buildid/c89b11207f6479603b0d49bf291c092c2b719293/debuginfo
query 1 urls in parallel
Progress 1 / 0
Progress 42 / 0
Progress 48 / 0
header HTTP/1.1 200 OK
header Date: Mon, 26 Aug 2024 11:37:05 GMT
header Server: Apache/2.4.6 (CentOS) OpenSSL/1.0.2k-fips mod_qos/11.70
header Cache-Control: public
header Last-Modified: Tue, 20 Sep 2022 15:27:27 GMT
header X-DEBUGINFOD-FILE: /usr/lib/debug/.build-id/c8/9b11207f6479603b0d49bf291c092c2b719293.debug
header X-DEBUGINFOD-ARCHIVE: /srv/mirror/debian-debug/main/c/coreutils/coreutils-dbgsym_9.1-1_amd64.deb
header X-DEBUGINFOD-SIZE: 143112
header Content-Type: application/octet-stream
header Vary: Accept-Encoding
header Content-Encoding: gzip
header Strict-Transport-Security: max-age=16070400
header Transfer-Encoding: chunked
header 
committed to url 0
Progress 8106 / -1
Progress 97162 / -1
server response No error
got file from server
found /home/user/.cache/debuginfod_client/c89b11207f6479603b0d49bf291c092c2b719293/debuginfo (fd=5)
Headers:
X-DEBUGINFOD-FILE: /usr/lib/debug/.build-id/c8/9b11207f6479603b0d49bf291c092c2b719293.debug
X-DEBUGINFOD-ARCHIVE: /srv/mirror/debian-debug/main/c/coreutils/coreutils-dbgsym_9.1-1_amd64.deb
X-DEBUGINFOD-SIZE: 143112
Downloaded from https://debuginfod.elfutils.org/buildid/c89b11207f6479603b0d49bf291c092c2b719293/debuginfo
/home/user/.cache/debuginfod_client/c89b11207f6479603b0d49bf291c092c2b719293/debuginfo

@florianl
Copy link
Contributor Author

With open-telemetry/opentelemetry-specification#4197 a specification was created for the use of build_ids in the context of OTel Profiles.

model/registry/process.yaml Outdated Show resolved Hide resolved
model/registry/process.yaml Outdated Show resolved Hide resolved
Co-authored-by: Christos Kalkanis <[email protected]>
florianl added a commit to florianl/opentelemetry-proto that referenced this pull request Sep 10, 2024
Drop `BuildIdKind` in favor of its semantic convention as defined in open-telemetry/semantic-conventions#1329.
Signed-off-by: Florian Lehner <[email protected]>
Copy link
Contributor

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

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

It can be done in a separate PR, but I think that https://github.com/open-telemetry/opentelemetry-specification/blob/a3248bfba81746acc1ba239cd98479cc1796b8e0/specification/profiles/mappings.md should exist in semantic conventions instead and be linked from the spec.

Reasons:

  • you describe attribute names and requirement levels there - semconv allow you do it formally + automate MD rendering and possibly code generation for the group(s) of profiling-related attribute.
  • you describe hashing algorithm there and anyone looking into semconv will need to go look into the spec. Same is true for the spec - you need to look into semconv anyway

So having everything described in details and in a formal way in semconv + having a short version in the spec would be easier to understand and navigate. Plus will have higher chance of staying consistent if any changes happen.

The group definition and markdown generation could be done in the same way as you already do for profile.frame

@lmolkova lmolkova merged commit 156daec into open-telemetry:main Sep 11, 2024
14 checks passed
florianl added a commit to open-telemetry/opentelemetry-ebpf-profiler that referenced this pull request Sep 12, 2024
open-telemetry/semantic-conventions#1329 got
merged, which introduces a semantic convention for build_ids. Use this
semantic convention in favor of BuildIdKind, that will be dropped with
open-telemetry/opentelemetry-proto#584.

Signed-off-by: Florian Lehner <[email protected]>
florianl added a commit to open-telemetry/opentelemetry-ebpf-profiler that referenced this pull request Sep 12, 2024
open-telemetry/semantic-conventions#1329 got
merged, which introduces a semantic convention for build_ids. Use this
semantic convention in favor of BuildIdKind, that will be dropped with
open-telemetry/opentelemetry-proto#584.

Signed-off-by: Florian Lehner <[email protected]>
tigrannajaryan added a commit to open-telemetry/opentelemetry-proto that referenced this pull request Sep 16, 2024
Drop `BuildIdKind` in favor of its semantic convention as defined in open-telemetry/semantic-conventions#1329.

Co-authored-by: Tigran Najaryan <[email protected]>
dmathieu pushed a commit to dmathieu/opentelemetry-proto that referenced this pull request Sep 23, 2024
Drop `BuildIdKind` in favor of its semantic convention as defined in open-telemetry/semantic-conventions#1329.

Co-authored-by: Tigran Najaryan <[email protected]>
drewby pushed a commit to drewby/otel-semantic-conventions that referenced this pull request Oct 1, 2024
Signed-off-by: Florian Lehner <[email protected]>
Co-authored-by: Christos Kalkanis <[email protected]>
Co-authored-by: Alexander Wert <[email protected]>
Co-authored-by: Liudmila Molkova <[email protected]>
tigrannajaryan pushed a commit to open-telemetry/opentelemetry-specification that referenced this pull request Oct 24, 2024
## Changes

Proposed change along with open-telemetry/semantic-conventions#1329 to clarify the use of `build_id` in the context of OTel Profiles.

FYI: @open-telemetry/profiling-maintainers 

* [x] [`CHANGELOG.md`](https://github.com/open-telemetry/opentelemetry-specification/blob/main/CHANGELOG.md) file updated for non-trivial changes
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
## Changes

Proposed change along with open-telemetry/semantic-conventions#1329 to clarify the use of `build_id` in the context of OTel Profiles.

FYI: @open-telemetry/profiling-maintainers 

* [x] [`CHANGELOG.md`](https://github.com/open-telemetry/opentelemetry-specification/blob/main/CHANGELOG.md) file updated for non-trivial changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

9 participants