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

symbolication: allow symbol upload for Bazel-built Go binairies #57

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

Gandem
Copy link
Member

@Gandem Gandem commented Sep 3, 2024

https://datadoghq.atlassian.net/browse/PROF-10434

Bazel built Go binairies have their GNU build ID stripped, and their Go build ID set to the string "redacted"
See https://github.com/bazelbuild/rules_go/blob/199d8e4827f87d382a85febd0148c1b42fa949cc/go/private/actions/link.bzl#L174

In that case, we want to able to rely on the file hash, for this reason we:

  • modify the function to get go build ID to consider that "redacted" is equivalent to not having a build ID
  • still upload debug symbols even if they don't contain a GNU build id or a Go build ID (they'll always contain a file hash, that we will be using for symbolication in that case).

The underlying goal is to allow remote symbolication with local symbol upload to work seamless for Go binaries built with Bazel

@DataDog/profiling-full-host

Bazel built Go binairies have their GNU build ID stripped, and
their Go build ID set to the string "redacted"
See https://github.com/bazelbuild/rules_go/blob/199d8e4827f87d382a85febd0148c1b42fa949cc/go/private/actions/link.bzl#L174

In that case, we want to able to rely on the file hash, for this
reason we:
- modify the function to get go build ID to consider that "redacted"
is equivalent to not having a build ID
- still upload debug symbols even if they don't contain a GNU build
id or a Go build ID (they'll always contain a file hash, that
we will be using for symbolication in that case).

The underlying goal is to allow remote symbolication with local
symbol upload to work seamless for Go binaries built with Bazel
@Gandem Gandem requested a review from a team as a code owner September 3, 2024 17:46
Copy link
Collaborator

@r1viollet r1viollet left a comment

Choose a reason for hiding this comment

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

LGTM
Happy that we identified this case.
Can we add some tests fpr this ? To start listing the binaries that we need to watch out for.

@Gandem
Copy link
Member Author

Gandem commented Sep 4, 2024

Can we add some tests for this ?

Yes we definitely should 💯 Though since the logic / package architecture might change a bit in the near future with:

  • the move to the agent as a library model
  • the addition of the API to query for IDs, as well as opening up symbol upload for more than DWARF

I would vote for investing in tests once we have those two bits in place - I'd prefer if we can merge this sooner to unblock symbolication for bazel-built Go binairies. I've captured this in https://datadoghq.atlassian.net/browse/PROF-10475. Let me know if that works for you!

@r1viollet
Copy link
Collaborator

That works! I was interested in adding some tests, though I did not know where to do so. Happy to resume this discussion in a sync.

@Gandem Gandem merged commit 5d1ecca into main Sep 4, 2024
15 checks passed
@Gandem Gandem deleted the nayef/build-id branch September 4, 2024 09:08
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