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

[cmd] Add new make targets with ldflags to build lite version binaries #34698

Merged
merged 2 commits into from
Sep 11, 2024

Conversation

Frapschen
Copy link
Contributor

Description:

We can use ldflags -w and -s to downscale the binary size. The official docs explanation of the two options:

-s
	Omit the symbol table and debug information.
-w
	Omit the DWARF symbol table.

After building with the above options, the binary size scales down 28%:

-rwxr-xr-x  1 fraps  staff   249M Aug 15 14:23 otelcontribcol_darwin_arm64
-rwxr-xr-x  1 fraps  staff   347M Aug  8 18:20 otelcontribcol_darwin_arm64-old

I think it's worth adding them. The only degradation is we can't use GDB to debug however it rare use case.

Link to tracking Issue:

Testing:

Documentation:

Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

Hello @Frapschen, I'm curious how this impacts end users, if you're able to provide more information.

  1. Does this change impact debugging with Delve?
  2. Is there any impact from this change on pprof?
  3. Are stacks logged by the zap.Logger or during a panic/crash impacted?

@Frapschen
Copy link
Contributor Author

Frapschen commented Aug 20, 2024

  1. Does this change impact debugging with Delve?
  2. Is there any impact from this change on pprof?
  3. Are stacks logged by the zap.Logger or during a panic/crash impacted?

1(2). Sure, pprof and Delve can't work if we add -w when building. see
3. not clearly

@crobert-1
Copy link
Member

We discussed this PR in the Collector sig meeting today, and the general consensus was that we should keep debugging symbols to make sure the pprof extension works properly by default. Users can build their own collector binaries without them if they'd prefer that.

If you want, you could even add new make targets with the modified flags proposed in this PR, that way it could be easier for users to accomplish this, but without changing the Collector's default build behavior. 👍

@andrzej-stencel
Copy link
Member

My opinion is that the binaries distributed by the project should include the debugging symbols. This does not block users from building their own binaries with the debug symbols excluded.

As a side note, this change does not modify the binaries published by the project at https://github.com/open-telemetry/opentelemetry-collector-releases/releases/. It only modifies the binaries that users build locally with make otelcontribcol.

@Frapschen Frapschen changed the title [cmd] Use -s and -w ldflags to downscale the binary size [cmd] Add new make targets with the -w -s ldflags to build lite version binaries Aug 23, 2024
@Frapschen Frapschen changed the title [cmd] Add new make targets with the -w -s ldflags to build lite version binaries [cmd] Add new make targets with ldflags to build lite version binaries Aug 23, 2024
@Frapschen
Copy link
Contributor Author

@crobert-1 as you suggest, I keep the Collector's default build behavior and add new target to build a lite version of binaries.

Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

I don't think we need a changelog entry for this, as it may be misleading to users. It may be worthwhile to add to a README somewhere, if there's anywhere that makes sense.

Makefile Outdated Show resolved Hide resolved
@crobert-1 crobert-1 added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Aug 23, 2024
@Frapschen
Copy link
Contributor Author

@crobert-1 PTAL thanks

Makefile.Common Outdated Show resolved Hide resolved
Co-authored-by: Curtis Robert <[email protected]>
Comment on lines +335 to +337
otelcontribcollite:
cd ./cmd/otelcontribcol && GO111MODULE=on CGO_ENABLED=0 $(GOCMD) build -trimpath -o ../../bin/otelcontribcol_$(GOOS)_$(GOARCH)$(EXTENSION) \
-tags $(GO_BUILD_TAGS) -ldflags $(GO_BUILD_LDFLAGS) .
Copy link
Member

Choose a reason for hiding this comment

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

There's a lot of repetition between otelcontribcol and otelcontribcollight targets. Perhaps it could be improved into something like:

Suggested change
otelcontribcollite:
cd ./cmd/otelcontribcol && GO111MODULE=on CGO_ENABLED=0 $(GOCMD) build -trimpath -o ../../bin/otelcontribcol_$(GOOS)_$(GOARCH)$(EXTENSION) \
-tags $(GO_BUILD_TAGS) -ldflags $(GO_BUILD_LDFLAGS) .
otelcontribcollite:
GO_BUILD_LDFLAGS="-s -w" $(MAKE) otelcontribcol

@andrzej-stencel andrzej-stencel merged commit 450eb67 into open-telemetry:main Sep 11, 2024
156 checks passed
@github-actions github-actions bot added this to the next release milestone Sep 11, 2024
jriguera pushed a commit to springernature/opentelemetry-collector-contrib that referenced this pull request Oct 4, 2024
open-telemetry#34698)

**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->

We can use ldflags `-w` and `-s` to downscale the binary size. The
official docs explanation of the two options:
```
-s
	Omit the symbol table and debug information.
-w
	Omit the DWARF symbol table.
```

After building with the above options, the binary size scales down 28%:
```
-rwxr-xr-x  1 fraps  staff   249M Aug 15 14:23 otelcontribcol_darwin_arm64
-rwxr-xr-x  1 fraps  staff   347M Aug  8 18:20 otelcontribcol_darwin_arm64-old
```

I think it's worth adding them. The only degradation is we can't use GDB
to debug however it rare use case.

**Link to tracking Issue:** <Issue number if applicable>

**Testing:** <Describe what testing was performed and which tests were
added.>

**Documentation:** <Describe the documentation added.>

---------

Co-authored-by: Curtis Robert <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants