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

Add 'pipeline' attribute to processor metrics #11171

Open
wants to merge 1 commit into
base: main
Choose a base branch
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.

Copy link

codecov bot commented Sep 13, 2024

Codecov Report

Attention: Patch coverage is 31.03448% with 20 lines in your changes missing coverage. Please review.

Project coverage is 91.80%. Comparing base (df3c9e3) to head (b8ddb2a).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
component/componenttest/otelchecker.go 0.00% 12 Missing ⚠️
component/componenttest/obsreporttest.go 0.00% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11171      +/-   ##
==========================================
- Coverage   91.87%   91.80%   -0.08%     
==========================================
  Files         421      422       +1     
  Lines       19989    20005      +16     
==========================================
  Hits        18365    18365              
- Misses       1249     1265      +16     
  Partials      375      375              

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

@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
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
component/componenttest/obsreporttest.go Show resolved Hide resolved
@@ -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 from 496326f to 08c4430 Compare September 19, 2024 20:11
@djaglowski djaglowski force-pushed the processor-pipeline-id-attribute branch from 08c4430 to b8ddb2a Compare September 19, 2024 20:15
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.

Processor telemetry should include Pipeline ID as attribute
2 participants