Skip to content

drop use of CGO in profiler core #628

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

Merged
merged 4 commits into from
Jul 19, 2025
Merged

Conversation

fabled
Copy link
Contributor

@fabled fabled commented Jul 16, 2025

This removes usage of CGO in the ebpf-profiler core by creating a cgo generated alias type for needed types with ebpf.

fixes #511

@fabled fabled force-pushed the tt-drop-cgo branch 2 times, most recently from dff06f9 to bb25a13 Compare July 16, 2025 20:36
@fabled fabled changed the title drop use of CGO drop use of CGO in profiler core Jul 16, 2025
@fabled fabled marked this pull request as ready for review July 16, 2025 20:55
@fabled fabled requested review from a team as code owners July 16, 2025 20:55
V8LineDeltaMask = C.V8_LINE_DELTA_MASK
)

var MetricsTranslation = []metrics.MetricID{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This brings the awkward dependency on metrics package, and has implications on metrics to not depend on libpf (which depends on support).

prevTimestamp libpf.UnixTime32
prevTimestamp uint32
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to avoid circular dependency of metrics -> libpf -> support -> metrics. This is because of moving the metrics mappings to the generated file in support (to avoid adding generated files).

Copy link
Member

Choose a reason for hiding this comment

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

We could also move UnixTime32 to times or util package but as we don't use the MarshalJSON method here so it's probably not worth it just for this case.

@@ -555,7 +555,7 @@ typedef struct Trace {
// Monotonic kernel time in nanosecond precision.
u64 ktime;
// The current COMM of the thread of this Trace.
char comm[COMM_LEN];
u8 comm[COMM_LEN];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This and the above CustomLabel change are due to arm64 and amd64 default char differing on signedness and would cause the generated code check to fail on one or the other flavor due to inconsistent type.

It also greatly simplifies things to have this unconditionall u8 which maps to uint8 (and for which byte is an alias). These are directly usable as []byte.

@@ -1165,6 +1077,7 @@ func (t *Tracer) probabilisticProfile(interval time.Duration, threshold uint) {
enableSampling := false
var probProfilingStatus = probProfilingDisable

//nolint:gosec
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to suppress lint warning on using non-secure random generator. This is ok here as this just for probability and not security related.

Copy link
Contributor

@florianl florianl left a comment

Choose a reason for hiding this comment

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

🎉

Copy link
Member

@christos68k christos68k left a comment

Choose a reason for hiding this comment

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

LGTM

prevTimestamp libpf.UnixTime32
prevTimestamp uint32
Copy link
Member

Choose a reason for hiding this comment

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

We could also move UnixTime32 to times or util package but as we don't use the MarshalJSON method here so it's probably not worth it just for this case.

@fabled fabled merged commit 24819cd into open-telemetry:main Jul 19, 2025
27 checks passed
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.

Building OTEL collector requires CGO_ENABLED=1
4 participants