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

RFC - Pipeline Component Telemetry #11406

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

djaglowski
Copy link
Member

@djaglowski djaglowski commented Oct 9, 2024

This PR adds a RFC for normalized telemetry across all pipeline components. See #11343

edit by @mx-psi:

Copy link

codecov bot commented Oct 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.61%. Comparing base (c6828f0) to head (cb72f2a).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #11406   +/-   ##
=======================================
  Coverage   91.61%   91.61%           
=======================================
  Files         443      443           
  Lines       23770    23770           
=======================================
  Hits        21776    21776           
  Misses       1620     1620           
  Partials      374      374           

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

@djaglowski djaglowski marked this pull request as ready for review October 10, 2024 13:36
@djaglowski djaglowski requested a review from a team as a code owner October 10, 2024 13:36
@djaglowski djaglowski added Skip Changelog PRs that do not require a CHANGELOG.md entry Skip Contrib Tests labels Oct 10, 2024
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks for opening this as a RFC @djaglowski!

docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
@djaglowski djaglowski changed the title RFC - Auto-instrumentation of pipeline components RFC - Pipeline Component Telemetry Oct 16, 2024
@djaglowski
Copy link
Member Author

Based on some offline feedback, I've broadened the scope of the RFC, while simultaneously clarifying that it is intended to evolve as we identify additional standards.

Copy link
Contributor

@jaronoff97 jaronoff97 left a comment

Choose a reason for hiding this comment

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

a few questions, I really like this proposal overall :)

docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Outdated Show resolved Hide resolved
@jpkrohling
Copy link
Member

Some of my comments might have been discussed before, in which case, feel free to ignore me and just mark the items as resolved.

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.

Approved with comments.

docs/rfcs/component-universal-telemetry.md Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Show resolved Hide resolved
@djaglowski djaglowski added the rfc:final-comment-period This RFC is in the final comment period phase label Nov 21, 2024
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

LGTM

codeboten added a commit to codeboten/opentelemetry-collector that referenced this pull request Nov 21, 2024
This sets the level of all metrics that where not previously stabilized as
alpha. Since many of these metrics will change as a result of
open-telemetry#11406, it made
sense to me to set their stability as alpha.

Signed-off-by: Alex Boten <[email protected]>
@mx-psi
Copy link
Member

mx-psi commented Nov 22, 2024

This has enough approvals and has entered the 'final comment period'. I will merge this on 2024-11-27 if nobody blocks before.

cc @open-telemetry/collector-approvers

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

There are a couple of things to iron out, but I'm giving my approval already, as those are details that could be part of a follow-up PR. I don't want to block progress on dependent tasks because of those two rather small points.

docs/rfcs/component-universal-telemetry.md Show resolved Hide resolved
docs/rfcs/component-universal-telemetry.md Show resolved Hide resolved
codeboten added a commit that referenced this pull request Nov 22, 2024
This sets the level of all metrics that where not previously stabilized
as alpha. Since many of these metrics will change as a result of
#11406, it
made sense to me to set their stability as alpha.

---------

Signed-off-by: Alex Boten <[email protected]>
@djaglowski
Copy link
Member Author

I believe all feedback has been addressed.

#11743 represents two followup items raised by @jpkrohling, but I believe the RFC is clear that some changes are anticipated.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks @djaglowski

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

I approved this before, but I'll approve again, to make it explicit that I'm OK with the latest state of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfc:final-comment-period This RFC is in the final comment period phase Skip Changelog PRs that do not require a CHANGELOG.md entry Skip Contrib Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.