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

Extract reporter data generation, and use pdata #245

Merged
merged 26 commits into from
Dec 10, 2024
Merged

Extract reporter data generation, and use pdata #245

merged 26 commits into from
Dec 10, 2024

Conversation

dmathieu
Copy link
Member

This is a proposal to help reduce the code repetition in #208
It extracts the data generation into its own internal module, and switches from OTLP to pdata (which is an OTLP abstraction), so both reporters can rely on the same data generator.

@dmathieu
Copy link
Member Author

This is blocked on open-telemetry/opentelemetry-collector#11706, since pdata currently uses a map for the attribute table.

@dmathieu dmathieu force-pushed the otlp-reporter-pdata branch from 3d87afb to 3b36dab Compare November 27, 2024 14:14
@dmathieu dmathieu marked this pull request as ready for review November 29, 2024 09:58
@dmathieu dmathieu requested review from a team as code owners November 29, 2024 09:58
@dmathieu
Copy link
Member Author

@open-telemetry/ebpf-profiler-approvers this change would greatly reduce the size of the OTLP and Collector reporters, as both would use pdata rather than OTLP and pdata.

florianl
florianl reviewed Nov 29, 2024
View reviewed changes
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.

Just some first spots. At the moment it is hard to review, as there is no backend that accepts this data. I will try to provide a devfiler version supporting v1.4.0 to continue here.

reporter/internal/pdata/generate.go Outdated Show resolved Hide resolved
reporter/otlp_reporter.go Outdated Show resolved Hide resolved
reporter/internal/pdata/generate.go Outdated Show resolved Hide resolved
reporter/otlp_reporter.go Outdated Show resolved Hide resolved
@dmathieu dmathieu force-pushed the otlp-reporter-pdata branch from d936928 to d4cf5d9 Compare November 29, 2024 13:34
@dmathieu dmathieu force-pushed the otlp-reporter-pdata branch from d4cf5d9 to 76589ca Compare November 29, 2024 13:37
@florianl
Copy link
Contributor

Here is a version of devfiler with OTel Profiling v1.4.0 support:

curl -L -H 'Authorization: ca14712d70ea6f6e' -o 'devfiler-v0.9.1-beta-v1.4.0.tar.gz' https://upload.elastic.co/d/b55a069766c48b8c0d4c0f2b854b0e846ee4deaea75596b68a8e1d88172dd0ca

or

https://upload.elastic.co/d/b55a069766c48b8c0d4c0f2b854b0e846ee4deaea75596b68a8e1d88172dd0ca
Authentication token: ca14712d70ea6f6e

@dmathieu dmathieu force-pushed the otlp-reporter-pdata branch from 13500da to 76e623a Compare December 2, 2024 09:35
@dmathieu dmathieu force-pushed the otlp-reporter-pdata branch from 76e623a to c2737da Compare December 2, 2024 09:35
@dmathieu
Copy link
Member Author

dmathieu commented Dec 4, 2024

@open-telemetry/ebpf-profiler-approvers this is ready for review.

@dmathieu

This comment was marked as resolved.

Sign in to view
florianl
florianl reviewed Dec 5, 2024
View reviewed changes
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.

Just a first impression. Will have a deeper look again.

reporter/internal/pdata/generate.go Show resolved Hide resolved
reporter/internal/pdata/generate.go Show resolved Hide resolved
reporter/internal/pdata/pdata.go Outdated Show resolved Hide resolved
@dmathieu dmathieu mentioned this pull request Dec 5, 2024
Closed
florianl
florianl approved these changes Dec 6, 2024
View reviewed changes
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.

Just a minor last comment. Approving as merging this allows to unblock other work. Minor things can still be changed in subsequent steps.

reporter/internal/pdata/generate.go Show resolved Hide resolved
@dmathieu dmathieu mentioned this pull request Dec 6, 2024
Open
christos68k
christos68k approved these changes Dec 6, 2024
View reviewed changes
reporter/internal/pdata/generate.go Outdated Show resolved Hide resolved
@dmathieu dmathieu force-pushed the otlp-reporter-pdata branch 6 times, most recently from 6d451a4 to bd69069 Compare December 6, 2024 17:34
@dmathieu dmathieu force-pushed the otlp-reporter-pdata branch from bd69069 to e1ef19b Compare December 6, 2024 17:36
rockdaboot
rockdaboot approved these changes Dec 9, 2024
View reviewed changes
Copy link
Contributor

@rockdaboot rockdaboot left a comment

Choose a reason for hiding this comment

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

LGTM

@rockdaboot rockdaboot merged commit 42bf4f5 into open-telemetry:main Dec 10, 2024
23 checks passed
@dmathieu dmathieu deleted the otlp-reporter-pdata branch December 10, 2024 09:57
@florianl florianl mentioned this pull request Dec 10, 2024
Merged
@dmathieu dmathieu mentioned this pull request Dec 10, 2024
Merged
@athre0z athre0z mentioned this pull request Dec 13, 2024
Closed
teresanovielloatinstana added a commit to instana/opentelemetry-ebpf-profiler that referenced this pull request Jan 17, 2025
@teresanovielloatinstana
Sync from upstream (2025-01-17) (#11)
e2ebf7c 
* Bump golangci-lint to 1.63.4 (open-telemetry#311)
  Author: Tim Rühsen <[email protected]>

* CI: reduce boiler plate of environment (open-telemetry#312)
* Consistent format of the eBPF code (open-telemetry#310)
* Off CPU profiling (open-telemetry#196)
* CI: Check for differences in the eBPF binary blobs (open-telemetry#305)
* README: drop make target docker-image as pre requirement (open-telemetry#304)
  Signed-off-by: Florian Lehner <[email protected]>

* Setup collector receiver (open-telemetry#160)
  Author: Damien Mathieu <[email protected]>
  Co-authored-by: Tim Rühsen <[email protected]>

* Send process name as process.executable.name (/proc/PID/comm) (open-telemetry#298)
  Author: Christos Kalkanis <[email protected]>
  Co-authored-by: Tim Rühsen <[email protected]>

* Remove host metadata collector (open-telemetry#299)
  Author: Damien Mathieu <[email protected]>

* apmint: return error if Attach() fails (open-telemetry#271)
* docker: build and push docker builder image (open-telemetry#297)
  Signed-off-by: Florian Lehner <[email protected]>

* Update golang.org/x/net to v0.33.0 (Fix CVE-2024-45338) (open-telemetry#294)
  Author: Tim Rühsen <[email protected]>

* metrics: update package documentation (open-telemetry#295)
  Signed-off-by: Florian Lehner <[email protected]>

* Fix pdata function table order (open-telemetry#286)
  Author: Tolya Korniltsev <[email protected]>

* Fix cross-compilation (open-telemetry#282) (open-telemetry#290)
  Author: Christos Kalkanis <[email protected]>

* Use clang-17 for building tracers (open-telemetry#270)
  Author: Mathieu Bressolle-Chataigner <[email protected]>
  Co-authored-by: Co-authored-by: Christos Kalkanis
<[email protected]>

* Make the CollAgentAddr config optional in controller (open-telemetry#279)
  Author: Damien Mathieu <[email protected]>
  Co-authored-by: Christos Kalkanis <[email protected]>
  Co-authored-by: Florian Lehner <[email protected]>

* Remove unused type libpf.TraceAndCounts (open-telemetry#283)
  Author: Tim Rühsen <[email protected]>

* Collector Reporter (open-telemetry#208)
  Author: Damien Mathieu <[email protected]>
  Co-authored-by: Tim Rühsen <[email protected]>
  Co-authored-by: Florian Lehner <[email protected]>
  Co-authored-by: Christos Kalkanis <[email protected]>

* proc: handle nvidia kernel modules (open-telemetry#274)
* README: provide new version of devfiler (open-telemetry#275)
  Author: Florian Lehner <[email protected]>

* Extract reporter data generation, and use pdata (open-telemetry#245)
  Author: Damien Mathieu <[email protected]>
  Co-authored-by: Florian Lehner <[email protected]>
  Co-authored-by: Christos Kalkanis <[email protected]>

* apmint: warn user about changes in the APM service (open-telemetry#269)
  Author: Florian Lehner <[email protected]>
  Co-authored-by: Joel Höner <[email protected]>

* coredump: no need for full dump with bundled files (open-telemetry#213)
  Author: Timo Teräs <[email protected]>

* Send executable path with every stack trace (open-telemetry#253)
* Send thread name as ThreadNameKey not ProcessCommandKey (open-telemetry#265)
  Author: Christos Kalkanis <[email protected]>

* proc: Fix parsing Kernel Module lines where refcount is - (open-telemetry#263)
  Author: Frederic Branczyk <[email protected]>

* reporter: extend lifetime for cached elements (open-telemetry#260)
  Author: Florian Lehner <[email protected]>

* Reporter:  allow extending samples with extra attributes (open-telemetry#237)
  Author: Joel Höner <[email protected]>
  Co-authored-by: Tim Rühsen <[email protected]>
florianl added a commit that referenced this pull request Jan 20, 2025
@florianl
reporter: move pkg samples from internal to public
9ec4a4c 
#245 refactored code and introduced a configuration option to an internal element. Promote package samples from an internal package to public again, so that ExtraSampleAttrProd can be configured again.

Fixes #280

Signed-off-by: Florian Lehner <[email protected]>
@florianl florianl mentioned this pull request Jan 20, 2025
Merged
teresanovielloatinstana added a commit to instana/opentelemetry-ebpf-profiler that referenced this pull request Jan 21, 2025
@teresanovielloatinstana
Sync from upstream (2025-01-17) (#11)
270db14 
* Bump golangci-lint to 1.63.4 (open-telemetry#311)
  Author: Tim Rühsen <[email protected]>

* CI: reduce boiler plate of environment (open-telemetry#312)
* Consistent format of the eBPF code (open-telemetry#310)
* Off CPU profiling (open-telemetry#196)
* CI: Check for differences in the eBPF binary blobs
  (open-telemetry#305)
* README: drop make target docker-image as pre requirement
  (open-telemetry#304)
  Signed-off-by: Florian Lehner <[email protected]>

* Setup collector receiver (open-telemetry#160)
  Author: Damien Mathieu <[email protected]>
  Co-authored-by: Tim Rühsen <[email protected]>

* Send process name as process.executable.name (/proc/PID/comm)
  (open-telemetry#298)
  Author: Christos Kalkanis <[email protected]>
  Co-authored-by: Tim Rühsen <[email protected]>

* Remove host metadata collector (open-telemetry#299)
  Author: Damien Mathieu <[email protected]>

* apmint: return error if Attach() fails (open-telemetry#271)
* docker: build and push docker builder image (open-telemetry#297)
  Signed-off-by: Florian Lehner <[email protected]>

* Update golang.org/x/net to v0.33.0 (Fix CVE-2024-45338)
  (open-telemetry#294)
  Author: Tim Rühsen <[email protected]>

* metrics: update package documentation (open-telemetry#295)
  Signed-off-by: Florian Lehner <[email protected]>

* Fix pdata function table order (open-telemetry#286)
  Author: Tolya Korniltsev <[email protected]>

* Fix cross-compilation (open-telemetry#282) (open-telemetry#290)
  Author: Christos Kalkanis <[email protected]>

* Use clang-17 for building tracers (open-telemetry#270)
  Author: Mathieu Bressolle-Chataigner <[email protected]>
  Co-authored-by: Co-authored-by: Christos Kalkanis
<[email protected]>

* Make the CollAgentAddr config optional in controller
  (open-telemetry#279)
  Author: Damien Mathieu <[email protected]>
  Co-authored-by: Christos Kalkanis <[email protected]>
  Co-authored-by: Florian Lehner <[email protected]>

* Remove unused type libpf.TraceAndCounts (open-telemetry#283)
  Author: Tim Rühsen <[email protected]>

* Collector Reporter (open-telemetry#208)
  Author: Damien Mathieu <[email protected]>
  Co-authored-by: Tim Rühsen <[email protected]>
  Co-authored-by: Florian Lehner <[email protected]>
  Co-authored-by: Christos Kalkanis <[email protected]>

* proc: handle nvidia kernel modules (open-telemetry#274)
* README: provide new version of devfiler (open-telemetry#275)
  Author: Florian Lehner <[email protected]>

* Extract reporter data generation, and use pdata (open-telemetry#245)
  Author: Damien Mathieu <[email protected]>
  Co-authored-by: Florian Lehner <[email protected]>
  Co-authored-by: Christos Kalkanis <[email protected]>

* apmint: warn user about changes in the APM service
  (open-telemetry#269)
  Author: Florian Lehner <[email protected]>
  Co-authored-by: Joel Höner <[email protected]>

* coredump: no need for full dump with bundled files
  (open-telemetry#213)
  Author: Timo Teräs <[email protected]>

* Send executable path with every stack trace (open-telemetry#253)
* Send thread name as ThreadNameKey not ProcessCommandKey
  (open-telemetry#265)
  Author: Christos Kalkanis <[email protected]>

* proc: Fix parsing Kernel Module lines where refcount is -
  (open-telemetry#263)
  Author: Frederic Branczyk <[email protected]>

* reporter: extend lifetime for cached elements (open-telemetry#260)
  Author: Florian Lehner <[email protected]>

* Reporter:  allow extending samples with extra attributes
  (open-telemetry#237)
  Author: Joel Höner <[email protected]>
  Co-authored-by: Tim Rühsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christos68k christos68k christos68k approved these changes

@rockdaboot rockdaboot rockdaboot approved these changes

@florianl florianl florianl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dmathieu @florianl @rockdaboot @christos68k