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

Add 'pipeline' attribute to processor metrics #11171

Closed
wants to merge 1 commit into from
Closed

Add 'pipeline' attribute to processor metrics #11171

wants to merge 1 commit into from

Conversation

djaglowski
Copy link
Member

Resolves #10908

This ends up being a pretty straightforward change in actual code, but a tedious change in tests. It's also obviously a breaking change for anyone depending on these metrics.

@codecov Codecov
Copy link

codecov bot commented Sep 13, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 60.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 91.40%. Comparing base (52e11ec) to head (23d20f7).

Files with missing lines Patch % Lines
service/pipelines/config.go 0.00% 3 Missing ⚠️
component/componenttest/obsreporttest.go 0.00% 2 Missing ⚠️
service/service.go 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #11171      +/-   ##
==========================================
- Coverage   91.47%   91.40%   -0.08%     
==========================================
  Files         424      424              
  Lines       20206    20207       +1     
==========================================
- Hits        18484    18470      -14     
- Misses       1339     1354      +15     
  Partials      383      383

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@djaglowski djaglowski force-pushed the processor-pipeline-id-attribute branch from 16e2224 to fef38a3 Compare September 13, 2024 19:38
@djaglowski djaglowski mentioned this pull request Sep 13, 2024
Closed
@djaglowski djaglowski force-pushed the processor-pipeline-id-attribute branch 2 times, most recently from f9d4226 to 9bece81 Compare September 13, 2024 20:39
@djaglowski
Copy link
Member Author

One notable detail is that the otel.signal attribute added in #11144 is arguably redundant, since pipeline ID's contain the data type. However, we likely will want otel.signal for receiver and exporter metrics as well, so perhaps it makes sense to leave it. (Connector metrics would need to differentiate between incoming and outgoing data type though.)

@djaglowski djaglowski force-pushed the processor-pipeline-id-attribute branch 2 times, most recently from 856197e to 0278ec4 Compare September 13, 2024 20:51
mx-psi
mx-psi reviewed Sep 16, 2024
View reviewed changes
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

The approach looks reasonable to me. There are some breaking changes on the test modules, I feel like those are okay (and that maybe we should split that part of the modules off to make our lives easier, but that's for another PR)

@djaglowski djaglowski force-pushed the processor-pipeline-id-attribute branch 2 times, most recently from 77300d1 to b1bf422 Compare September 16, 2024 13:28
@djaglowski
Copy link
Member Author

The approach looks reasonable to me. There are some breaking changes on the test modules, I feel like those are okay (and that maybe we should split that part of the modules off to make our lives easier, but that's for another PR)

I added a changelog to note to breaking change in processortest.NewNopSettings

@djaglowski
Copy link
Member Author

I opened #11179 to capture thoughts on the broader question of which attributes to apply to each kind of component. This PR aligns with what I believe is the best overall schema, but please take a look.

@djaglowski djaglowski marked this pull request as ready for review September 16, 2024 15:26
@djaglowski djaglowski requested review from a team and TylerHelmuth September 16, 2024 15:26
@djaglowski djaglowski force-pushed the processor-pipeline-id-attribute branch 3 times, most recently from 8efa7be to 496326f Compare September 19, 2024 13:38
@djaglowski djaglowski requested a review from a team as a code owner September 19, 2024 13:38
mx-psi
mx-psi reviewed Sep 19, 2024
View reviewed changes
component/componenttest/obsreporttest.go Show resolved Hide resolved
processor/processortest/nop_processor.go Outdated
@@ -20,11 +20,12 @@ import (
var nopType = component.MustNewType("nop")

// NewNopSettings returns a new nop settings for Create*Processor functions.
func NewNopSettings() processor.Settings {
func NewNopSettings(dt component.DataType) processor.Settings {
Copy link
Member

Choose a reason for hiding this comment

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

Any thoughts on whether we should require this argument for other test packages, just for consistency? The situation I am worried about is that we eventually want to pass the pipeline ID for some reason (not necessarily self telemetry) for all components and we end up having to change it everywhere

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems likely we'll want to include the data type for other component kinds. I suppose we could pass the pipeline ID here for processors, but not sure there's any case to do it for other kinds. This is probably something worth trying to iron out as part of #11179.

@djaglowski djaglowski force-pushed the processor-pipeline-id-attribute branch 3 times, most recently from b8ddb2a to d47db79 Compare September 24, 2024 13:52
bogdandrutu
bogdandrutu reviewed Sep 28, 2024
View reviewed changes
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Is this already done?

processor/internal/processor.go Outdated
Comment on lines 21 to 22
// PipelineID indicates which pipeline contains this processor.
PipelineID pipeline.ID
Copy link
Member

Choose a reason for hiding this comment

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

Settings are passed to the factory method correct? Which already signals which "signal" is this for. I am confused why adding it here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This indicates the pipeline ID, not the signal. The rationale for adding pipeline ID is explained in #10908

@djaglowski djaglowski force-pushed the processor-pipeline-id-attribute branch from d47db79 to 23d20f7 Compare September 30, 2024 13:48
@djaglowski djaglowski mentioned this pull request Sep 30, 2024
Closed
@bogdandrutu
Copy link
Member

Please rebase, looks like everything changed since this change was made.

@djaglowski
Copy link
Member Author

I'm closing this in favor of the graph based approach, demonstrated in #11311 (review) and more robustly proposed in #11343.

@djaglowski djaglowski closed this Oct 9, 2024
@djaglowski djaglowski deleted the processor-pipeline-id-attribute branch October 9, 2024 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mx-psi mx-psi mx-psi approved these changes

@TylerHelmuth TylerHelmuth Awaiting requested review from TylerHelmuth

@bogdandrutu bogdandrutu Awaiting requested review from bogdandrutu

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

Successfully merging this pull request may close these issues.

Processor telemetry should include Pipeline ID as attribute
3 participants
@djaglowski @bogdandrutu @mx-psi