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.

Sign up for GitHub

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

Jump to bottom

Enabling debug symbols #58

Closed
aateeqi opened this issue Jan 14, 2022 · 8 comments · Fixed by open-telemetry/opentelemetry-collector#11996
Closed

Enabling debug symbols #58

aateeqi opened this issue Jan 14, 2022 · 8 comments · Fixed by open-telemetry/opentelemetry-collector#11996

Comments

@aateeqi
Copy link
Contributor

aateeqi commented Jan 14, 2022

We are contemplating whether to enable debug symbols (disabled by -s -w LDFLAGS on build) on our downstream repo aws-otel-collector. We want to be in line with what is being done upstream but it seems that there's an inconsistency. In this opentelemetry-collector-releases repo it appears that they are disabled but in opentelemetry-collector-contrib it appears to be enabled.

Is there any reason for this inconsistence/any guidance on what we should lean towards regarding enabling/disabling debug symbols?
@Aneurysm9 Aneurysm9 mentioned this issue Jan 14, 2022
Closed
@jpkrohling
Copy link
Member

Are there performance penalties in having debug symbols enabled?

@jpkrohling jpkrohling mentioned this issue Jan 25, 2022
Closed
@jpkrohling
Copy link
Member

@kaero, @aateeqi, would you be open to opening a PR for this?

@kaero
Copy link

kaero commented Jan 25, 2022

@jpkrohling

@kaero, @aateeqi, would you be open to opening a PR for this?

Yes, I would like to implement the feature in the builder in a backward-compatible way as it suggested in #63 (comment) , but I have no idea who and how use releases issued from this repository. We make private builds of the collector using the builder.

I could update builder manifests here too if it would be helpful for users of those releases.

@kaero
Copy link

kaero commented Jan 25, 2022

Are there performance penalties in having debug symbols enabled?

Generally speaking, no. Debug symbols are used by debugging facilities in cases they required to, for example when a user attach or run process under a debugger or the Linux perf tool. The only effect everyone who don't care about debugging should notice is the growth of the binary size.

@jpkrohling
Copy link
Member

I believe the only place you'd need to change is this:

opentelemetry-collector-releases/goreleaser/configure.go

Line 76 in 337b166

Ldflags: []string{"-s", "-w"},

@kaero
Copy link

kaero commented Jan 25, 2022

I believe the only place you'd need to change is this:

I don't see, how it would help binaries built using the OpenTelemetry builder command line tool.

@jpkrohling
Copy link
Member

Sorry, I missed that part!

We make private builds of the collector using the builder.

This would be the place to remove it:
https://github.com/open-telemetry/opentelemetry-collector/blob/88e9c86f2d32c224a488a5a4935cdafcafc5b943/cmd/builder/internal/builder/main.go#L89

I think we can externalize it in a config property.

@dosubot dosubot bot added the stale Issue has not had recent activity or appears to be solved. Stale issues will be automatically closed label Jul 16, 2024
@dosubot dosubot bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 23, 2024
@dosubot dosubot bot removed the stale Issue has not had recent activity or appears to be solved. Stale issues will be automatically closed label Jul 23, 2024
@mx-psi mx-psi reopened this Jul 23, 2024
@Dainerx
Copy link

Dainerx commented Dec 18, 2024 •
edited
Loading

I think this issue is still relevant today. Looking at the builder code, we can override ld flags only when debug compilation property is set to true. Is there a way to only override ld flags?

The purpose here is to keep debug symbols and DWARF data without setting those GCflags.

@Dainerx Dainerx mentioned this issue Dec 30, 2024
Merged
github-merge-queue bot pushed a commit to open-telemetry/opentelemetry-collector that referenced this issue Jan 21, 2025
@Dainerx
[chore] Ability to provide custom ld and gc flags (#11996)
24da249 
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

The primary purpose of this PR is to provide greater flexibility in how
OTEL binaries are built, enabling the inclusion of debugging symbols
when needed, without always stripping them by default.

Currently, debugging symbols are only retained when
debug_compilation=true. However, this approach also disables all
compiler inlining and optimizations (gcflags=all=-N -l) to ensure an
exact match between written and executed code, resulting in a
significant increase in CPU consumption. There are scenarios where we
want binaries with debugging symbols and DWARF information while still
allowing the compiler to optimize and inline. This PR addresses that
need by introducing configurable GCFlags.

`ocb --ldflags="" --gcflags="" --config=builder-config.yaml`

<!-- Issue number if applicable -->
#### Link to tracking issue
Fixes
[#58](open-telemetry/opentelemetry-collector-releases#58)

<!--Describe what testing was performed and which tests were added.-->
#### Testing

Manual 

Override LDflags: 


![image](https://github.com/user-attachments/assets/6bcb0f7b-492a-45fb-a232-9c337afb5f5e)


Override both


![image](https://github.com/user-attachments/assets/00bc6c5f-37f0-438d-b4e1-f7a2d5833ec9)


<!--Describe the documentation added.-->
#### Documentation

README file updated.

-- 

Backward compatibility concerns: 
- As of today, passing cfg.LDFlags will append to LD flags that are by
default to `-s -w`.

Questions: 
- Should we deprecate DebugCompilation property? 
<!--Please delete paragraphs that you did not use before submitting.-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[chore] Ability to provide custom ld and gc flags Dainerx/opentelemetry-collector
5 participants
@jpkrohling @kaero @mx-psi @Dainerx @aateeqi