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

Extract reporter data generation, and use pdata #245

Merged
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.

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.

@dmathieu dmathieu force-pushed the otlp-reporter-pdata branch from 128b9ea to d1c49f2 Compare December 4, 2024 11:16
@dmathieu dmathieu marked this pull request as ready for review December 4, 2024 11:42
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.

Did a pass, looks good to me. I'll spend a little more time with generate.go tomorrow.

reporter/internal/pdata/pdata.go Outdated Show resolved Hide resolved
reporter/internal/pdata/pdata.go Outdated Show resolved Hide resolved
reporter/internal/pdata/pdata.go Outdated Show resolved Hide resolved
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
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
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
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
@dmathieu dmathieu mentioned this pull request Dec 10, 2024
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.

4 participants