-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
RFC: Experimental profiling #10375
RFC: Experimental profiling #10375
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10375 +/- ##
=======================================
Coverage 92.36% 92.36%
=======================================
Files 386 386
Lines 18400 18400
=======================================
Hits 16995 16995
Misses 1052 1052
Partials 353 353 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dmathieu, this looks great. Just one comment
This comment was marked as resolved.
This comment was marked as resolved.
Co-authored-by: Christos Kalkanis <christos.kalkanis@elastic.co>
Co-authored-by: Christos Kalkanis <christos.kalkanis@elastic.co>
Co-authored-by: Christos Kalkanis <christos.kalkanis@elastic.co>
Co-authored-by: Christos Kalkanis <christos.kalkanis@elastic.co>
Co-authored-by: Christos Kalkanis <christos.kalkanis@elastic.co>
4423f8e
to
13caa79
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can merge this as is and as noted on the Collector SIG I believe we can merge this on Friday.
One remaining step is what to do with service and <receiver|exporter|connector|processor>.Builder
. The problem is that we don't pass the configuration directly to the service and instead pass Builders
. These builders encapsulate the factories and config, thus we lose access to the config.
We will need to solve this problem during implementation, some options are:
- Add a
Config
method to builders similar to theFactory
method - Make builders internal to the service and pass the config and factories explicitly (honestly at this point my preferred option, there is no real abstraction/usefulness to have these exposed)
- Make the builders into sealed interfaces like with the factory interfaces? (not sure if there's much of a point for this)
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description This follows #10530, and adds profiles support into receivers. <!-- Issue number if applicable --> #### Link to tracking issue See #10375 cc @mx-psi --------- Co-authored-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>
Following a discussion with @mx-psi this morning, we have decided to follow option 2 mentioned above for the builders. |
I have pasted this in a couple PRs, pasting it here as well:
|
Sure, I've done that in all 5 PRs. |
#### Description This moves the receiver builder out of the `receiver` package, and into `service/internal/builders`. There's no real reason for this struct to be public (folks shouldn't call it), and making it private will allow us to add profiling support to it. #### Link to tracking issue #10375 (review)
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description This moves the connector builder out of the `connector` package, and into `service/internal/builders`. There's no real reason for this struct to be public (folks shouldn't call it), and making it private will allow us to add profiling support to it. <!-- Issue number if applicable --> #### Link to tracking issue #10375 (review)
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description This moves the connector builder out of the `connector` package, and into `service/internal/builders`. There's no real reason for this struct to be public (folks shouldn't call it), and making it private will allow us to add profiling support to it. <!-- Issue number if applicable --> #### Link to tracking issue #10375 (review) While this is not technically required for the profiles work (there is no notion of signals in extensions), this PR is here to keep things consistent.
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description This moves the exporter builder out of the `exporter` package, and into `service/internal/builders`. There's no real reason for this struct to be public (folks shouldn't call it), and making it private will allow us to add profiling support to it. <!-- Issue number if applicable --> #### Link to tracking issue #10375 (review)
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description This moves the processor builder out of the `processor` package, and into `service/internal/builders`. There's no real reason for this struct to be public (folks shouldn't call it), and making it private will allow us to add profiling support to it. <!-- Issue number if applicable --> #### Link to tracking issue #10375 (review)
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description This adds profiles support in fanoutconsumer. That is required to be able to handle profiles in the service graph. As this is only changing internal modules, I am marking it as chore (and skipping changelog entry). <!-- Issue number if applicable --> #### Link to tracking issue #10375 cc @mx-psi
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description This allows building profiles in the receiver builder. As this is only changing internal modules, I am marking it as chore (and skipping changelog entry). <!-- Issue number if applicable --> #### Link to tracking issue #10375 cc @mx-psi
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description This allows building profiles in the processor builder. As this is only changing internal modules, I am marking it as chore (and skipping changelog entry). #### Link to tracking issue #10375 cc @mx-psi
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description This allows building profiles in the connector builder. As this is only changing internal modules, I am marking it as chore (and skipping changelog entry). #### Link to tracking issue #10375 cc @mx-psi
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description This allows building profiles in the exporter builder. As this is only changing internal modules, I am marking it as chore (and skipping changelog entry). #### Link to tracking issue #10375 cc @mx-psi
Description
This is an RFC discussing the proposed solution for adding the profiling signal.
Follows #10207
PoC: #10307
cc @open-telemetry/profiling-triagers @open-telemetry/profiling-approvers @open-telemetry/profiling-maintainers
cc @open-telemetry/collector-approvers