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

[processorhelper] deprecating unused methods #11083

Merged

Conversation

codeboten
Copy link
Contributor

[Logs|Traces|Metrics]Inserted was not used in the core/contrib repos, marking them as deprecated.

*Inserted was not used in the core/contrib repos, marking them as deprecated.

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@codeboten codeboten marked this pull request as ready for review September 9, 2024 16:41
@codeboten codeboten requested review from a team and djaglowski September 9, 2024 16:41
Copy link

codecov bot commented Sep 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.20%. Comparing base (6928951) to head (3b7a30c).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11083      +/-   ##
==========================================
- Coverage   92.21%   92.20%   -0.01%     
==========================================
  Files         414      414              
  Lines       19802    19804       +2     
==========================================
  Hits        18261    18261              
- Misses       1168     1170       +2     
  Partials      373      373              

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

@codeboten codeboten merged commit 3f9bc53 into open-telemetry:main Sep 9, 2024
49 of 66 checks passed
@codeboten codeboten deleted the codeboten/deprecate-unused-methods branch September 9, 2024 17:09
@github-actions github-actions bot added this to the next release milestone Sep 9, 2024
dmathieu pushed a commit to dmathieu/opentelemetry-collector that referenced this pull request Sep 10, 2024
`[Logs|Traces|Metrics]Inserted` was not used in the core/contrib repos,
marking them as deprecated.

---------

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@djaglowski
Copy link
Member

The rationale for these metrics was described in #10353.

They are complimentary to the Dropped metrics in that they can account for items which were added rather than removed by a processor. For example, a processor which computes a metric based on two others would record that it has inserted data points. Arguably, aggregation should be described as both dropping and inserting data. e.g. 10 log records replaced by 1.

In any case, Accepted, Refused, and Dropped are used only where component authors have manually recorded them so in my opinion we should keep or deprecate all of them together.

@codeboten
Copy link
Contributor Author

In any case, Accepted, Refused, and Dropped are used only where component authors have manually recorded them so in my opinion we should keep or deprecate all of them together.

I agree. Looking through the other metrics, the only processor that makes use of them is the memory limiter processor. I'm not against the idea that processors should report this information. I thought it would make sense to report them via the incoming/outgoing metrics that were added in #10910 with the otel.outcome attribute for being the differentiator as suggested in open-telemetry/oteps#259

@djaglowski
Copy link
Member

I thought it would make sense to report them via the incoming/outgoing metrics that were added in #10910 with the otel.outcome attribute for being the differentiator as suggested in open-telemetry/oteps#259

I'm certainly in favor of this - just suggesting that we should be keeping or deprecating these metrics as wholistic sets, whichever we choose.

@codeboten
Copy link
Contributor Author

@djaglowski do you want to open an issue to migrate the existing processors to using incoming/outgoing w/ outcome attributes and we can reference that issue as we make changes to the telemetry

@djaglowski
Copy link
Member

I have to walk back my statement as meaning to apply only to the incoming/outgoing metrics. I'm not opposed to the outcome idea but I don't understand how we could automatically record this attribute.

@codeboten
Copy link
Contributor Author

it wouldn't need to be automatic.... the processor could use processorhelper functions to record the outgoing metric w/ the correct attribute. In other words, maybe the funcs for recording the old metrics are deprecated in favour of new funcs that match the new metric?

@djaglowski
Copy link
Member

it wouldn't need to be automatic....

For context, the incoming/outgoing metrics were added based on #10708 which specifically calls out automatically recorded metrics.

IMO this is critical. There's going to be very little value in any metrics that must be recorded manually by each and every processor, especially when there's so much nuance to their meaning that the OTEP has gone stale before leaving draft state.

I do think there is a simple version of the outcome dimension that can be automatically recorded based purely on whether the next consumer returned an error, but this is quite different than trying to match the OTEP.

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.

3 participants