Donating ebpf based instrumentation #4

edeNFed · 2022-08-03T18:24:27Z

This PR contains a copy of all the files in the https://github.com/keyval-dev/opentelemetry-go-instrumentation repository

tigrannajaryan · 2022-08-03T18:41:32Z

Please make sure to follow the attribution guidelines. All source code files should carry the corresponding copyright notice. If you copied any files from other open-source projects that they need to carry the attribution notice.

Also, is ebpf-based/vmlinux.h generated or it is a copy from somewhere else? If generated please include generation instruction, if copied it needs an attribution.

edeNFed · 2022-08-04T07:32:45Z

Added. I followed the same licenses that cilium (another eBPF based CNCF project) use for their Go code / eBPF programs.

ebpf-based/Makefile

tigrannajaryan · 2022-10-19T14:09:57Z

Added. I followed the same licenses that cilium (another eBPF based CNCF project) use for their Go code / eBPF programs.

@edeNFed Thank you. To help with the review, can you please tell which files are derived/copied from elsewhere and which files are authored by yourself?

For files that are derived/copied we will need to know their original licenses, since not every license is allowed.

For files that you are the author are you the sole author or you are contributing this on behalf of other authors / on behalf of a company?

ebpf-based/include/arguments.h

edeNFed · 2022-10-31T09:34:27Z

@tigrannajaryan The files in include/libbpf directory are copied from the linux kernel: https://github.com/torvalds/linux/tree/master/tools/lib/bpf
All the other files are written by myself and are donated on behalf of Keyval.
Please let me know if there are still attribution issues.

@open-telemetry/go-instrumentation-approvers would love to get a review for this PR.

pellared · 2022-10-31T09:52:26Z

@tigrannajaryan The files in include/libbpf directory are copied from the linux kernel: https://github.com/torvalds/linux/tree/master/tools/lib/bpf All the other files are written by myself and are donated on behalf of Keyval. Please let me know if there are still attribution issues.

@open-telemetry/go-instrumentation-approvers would love to get a review for this PR.

The libbpf is dual-licensed. IANAL but I have no idea how we should say that we are "choosing" the BSD-2-Clause as LGPL-2.1 may be problematic, because of:

Each time you redistribute the Library (or any work based on the Library), the recipient automatically receives a license from the original licensor to copy, distribute, link with or modify the Library subject to these terms and conditions.

Any ideas?

edeNFed · 2022-10-31T10:04:15Z

Those files are needed for easy linkage during compilation. I am not sure if this makes the decision easier but I guess we can download them during the compilation script instead of having a copy in this repo.

Anyway, I think there should be a way to use those files as other eBPF based projects in the CNCF (cilium for example) are also dependent on them.

pellared · 2022-10-31T12:19:40Z

Those files are needed for easy linkage during compilation. I am not sure if this makes the decision easier but I guess we can download them during the compilation script instead of having a copy in this repo.

It is not about "storing" the code but the fact that the library is linked.

Reference: https://www.gnu.org/licenses/old-licenses/lgpl-2.1.en.html

Anyway, I think there should be a way to use those files as other eBPF based projects in the CNCF (cilium for example) are also dependent on them.

That does not guarantee that these repositories are OK from legal perspective 😉

For instance, AFAIK, MIT requires adding a copywrite notice / EULA.

pellared · 2022-10-31T13:33:11Z

My "perfectionist ego" forced me to create open-telemetry/community#1280 to track the issue from the OTel organization perspective. I hope nobody is going to hate me for it 😉

I think we should consider deferring the licensing "issue" I brought (we can create an issue for it).

At this point of time I think it is fine to use libbpf using the "precedence rule".

tigrannajaryan · 2022-11-03T21:09:17Z

BSD is explicitly allowed by Otel provided that the notice is retained which it is is, so we are good: https://github.com/open-telemetry/community/blob/main/CONTRIBUTING.md#copyright-notices

damemi

@edeNFed, this is a really cool project. Separating each library the way you have done gives it a neat modular-manager design that makes this easily extensible in the future.

I left some comments for future ideas but I am +1 to merging this if there's nothing blocking us (mainly because I want to get my hands dirty and start contributing myself!)

include/utils.h

damemi · 2022-11-11T16:05:15Z

cli/main.go

+	"github.com/open-telemetry/opentelemetry-go-instrumentation/pkg/process"
+)
+
+func main() {


follow-up: help/usage text would be useful

damemi · 2022-11-11T16:08:23Z

pkg/process/args.go

+	return nil
+}
+
+func ParseTargetArgs() *TargetArgs {


any consideration for command line flags? Or at least a centralized struct for storing the various required arguments (process exe, collector endpoint, service name)

Good idea, the current implementation tries to follow: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md

damemi · 2022-11-11T16:10:34Z

pkg/instrumentors/manager.go

+	insts := []Instrumentor{
+		grpc.New(),
+		grpcServer.New(),
+		httpServer.New(),
+		gorillaMux.New(),
+	}


I like how modular this is, it looks like it will make adding support for new libraries fairly easy in the future. wonder if this could eventually be made customizable rather than hard-coded for people to support their own libraries

damemi · 2022-11-11T16:25:41Z

pkg/inject/injector.go

+}
+
+func (i *Injector) getFieldOffset(libName string, libVersion string, structName string, fieldName string) (uint64, bool) {
+	for _, l := range i.data.Data {


From the docs:

Luckily for us, there is a way to analyze the target binary and extract the required offsets, by using DWARF. The DWARF debug information is generated by the compiler and is stored inside the binary.
Notice that one of our design goals is to support stripped Go binaries - meaning binaries that do not contain debug information. In order to support stripped binaries and to create a stable instrumentation, we created a library called offsets-tracker.

does this mean that in non-stripped binaries, we could retrieve, store, and access the offset data without needing to do this loop?

I also wonder if there are ways to optimize this data structure, maybe something we could look into if it saves a significant amount of time

Yes, I agree. this loop can probably be skipped for non-stripped binaries.
Also +1 for creating a better data structure.

pellared

Leaving just one comment to make sure that this donation is safe from the legal perspective.

include/libbpf/bpf_endian.h

pellared · 2022-11-15T16:10:54Z

I am approving as I think that this PR is too big to process it in a regular way. I suggest merging it as it is. People can always look at this PR and add comments. We may address comments as separate PRs

dineshg13

As discussed at SIG meeting , we should be good to merge this .

Co-authored-by: Robert Pająk <[email protected]>

Define the CODEOWNERS file as owned by the project maintainers and all else owned by the approvers. This define the required reviews for modification of the owned files.

Check the GitHub actions for updates weekly.

* Remove extra paren from link in README * Fix license link

Co-authored-by: Robert Pająk <[email protected]>

Merge upstream main, add changelog and make small fixes

edeNFed mentioned this pull request Aug 3, 2022

Donation proposal: OpenTelemetry Go Automatic Instrumentation #2

Closed

dineshg13 reviewed Oct 18, 2022

View reviewed changes

ebpf-based/Makefile Outdated Show resolved Hide resolved

tigrannajaryan reviewed Oct 19, 2022

View reviewed changes

ebpf-based/include/arguments.h Outdated Show resolved Hide resolved

edeNFed requested a review from a team October 23, 2022 14:50

edeNFed marked this pull request as draft October 23, 2022 14:52

edeNFed marked this pull request as ready for review October 31, 2022 09:30

damemi approved these changes Nov 11, 2022

View reviewed changes

pellared requested changes Nov 15, 2022

View reviewed changes

include/libbpf/bpf_endian.h Outdated Show resolved Hide resolved

edeNFed requested a review from pellared November 15, 2022 14:55

pellared approved these changes Nov 15, 2022

View reviewed changes

dineshg13 approved these changes Nov 15, 2022

View reviewed changes

pdelewski approved these changes Nov 15, 2022

View reviewed changes

jmacd and others added 8 commits November 16, 2022 09:04

Initial commit

96ce098

copy files from original repo

bfeef40

Add attribution

2cc2e70

Add CONTRIBUTING.md

e1d202c

Apply suggestions from code review

4e6a518

Co-authored-by: Robert Pająk <[email protected]>

Add project CODEOWNERS file

e0468d9

Define the CODEOWNERS file as owned by the project maintainers and all else owned by the approvers. This define the required reviews for modification of the owned files.

Add README.md (#8)

95fba6a

Add dependabot configuration (#13)

569923b

Check the GitHub actions for updates weekly.

MrAlias and others added 8 commits November 16, 2022 09:06

Fix link formting in README (#14)

d42150e

* Remove extra paren from link in README * Fix license link

merge latest version from keyval

467351d

Add bug and feature request issue templates (#12)

3529034

Co-authored-by: Robert Pająk <[email protected]>

remove generated files

ee44016

add attribution

6f3838c

change module name to open-telemetry org

e7e6718

fix newline at end of files

27cebae

add license files

4812f27

edeNFed closed this Nov 16, 2022

edeNFed force-pushed the main branch from 10aeb12 to 4812f27 Compare November 16, 2022 07:07

edeNFed mentioned this pull request Nov 16, 2022

eBPF based instrumentation #15

Merged

MikeGoldsmith pushed a commit that referenced this pull request Jun 14, 2023

Merge pull request #4 from honeycombio/oxeye.http_client_instrumentation

d59e91b

Merge upstream main, add changelog and make small fixes

damemi mentioned this pull request Feb 28, 2024

License Scan/Remediation Questions #697

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Donating ebpf based instrumentation #4

Donating ebpf based instrumentation #4

edeNFed commented Aug 3, 2022

tigrannajaryan commented Aug 3, 2022

edeNFed commented Aug 4, 2022

tigrannajaryan commented Oct 19, 2022

edeNFed commented Oct 31, 2022

pellared commented Oct 31, 2022

edeNFed commented Oct 31, 2022

pellared commented Oct 31, 2022

pellared commented Oct 31, 2022 •

edited

Loading

tigrannajaryan commented Nov 3, 2022 •

edited

Loading

damemi left a comment

damemi Nov 11, 2022

damemi Nov 11, 2022

edeNFed Nov 13, 2022

damemi Nov 11, 2022

damemi Nov 11, 2022

edeNFed Nov 13, 2022

pellared left a comment

pellared commented Nov 15, 2022

dineshg13 left a comment •

edited

Loading

Donating ebpf based instrumentation #4

Donating ebpf based instrumentation #4

Conversation

edeNFed commented Aug 3, 2022

tigrannajaryan commented Aug 3, 2022

edeNFed commented Aug 4, 2022

tigrannajaryan commented Oct 19, 2022

edeNFed commented Oct 31, 2022

pellared commented Oct 31, 2022

edeNFed commented Oct 31, 2022

pellared commented Oct 31, 2022

pellared commented Oct 31, 2022 • edited Loading

tigrannajaryan commented Nov 3, 2022 • edited Loading

damemi left a comment

Choose a reason for hiding this comment

damemi Nov 11, 2022

Choose a reason for hiding this comment

damemi Nov 11, 2022

Choose a reason for hiding this comment

edeNFed Nov 13, 2022

Choose a reason for hiding this comment

damemi Nov 11, 2022

Choose a reason for hiding this comment

damemi Nov 11, 2022

Choose a reason for hiding this comment

edeNFed Nov 13, 2022

Choose a reason for hiding this comment

pellared left a comment

Choose a reason for hiding this comment

pellared commented Nov 15, 2022

dineshg13 left a comment • edited Loading

Choose a reason for hiding this comment

pellared commented Oct 31, 2022 •

edited

Loading

tigrannajaryan commented Nov 3, 2022 •

edited

Loading

dineshg13 left a comment •

edited

Loading