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

[configtelemetry] add guidelines for each level of config telemetry #10365

Merged
merged 22 commits into from
Aug 28, 2024

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Jun 7, 2024

Description

Add to the package comment a set of guidelines for configtelemetry levels.

Link to tracking issue

Fixes #10286

@atoulme atoulme requested review from a team and songy23 June 7, 2024 07:23
@atoulme atoulme force-pushed the add_guidelines_telemetry branch from f36508e to f576a26 Compare June 7, 2024 07:27
Copy link

codecov bot commented Jun 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.86%. Comparing base (343f449) to head (33fea7b).
Report is 31 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10365      +/-   ##
==========================================
+ Coverage   91.62%   91.86%   +0.24%     
==========================================
  Files         406      411       +5     
  Lines       19046    19328     +282     
==========================================
+ Hits        17450    17756     +306     
+ Misses       1237     1221      -16     
+ Partials      359      351       -8     

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

@atoulme
Copy link
Contributor Author

atoulme commented Jun 7, 2024

@jaronoff97 ptal

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 thoughts, i like where this is heading.

config/configtelemetry/doc.go Outdated Show resolved Hide resolved
config/configtelemetry/doc.go Outdated Show resolved Hide resolved
config/configtelemetry/doc.go Outdated Show resolved Hide resolved
config/configtelemetry/doc.go Outdated Show resolved Hide resolved
@jaronoff97
Copy link
Contributor

I do think in the future it would be neat if we had wrapper methods around otel APIs for recording telemetry for each level so authors don't need to keep track of these things.

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jun 22, 2024
@atoulme atoulme removed the Stale label Jun 24, 2024
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added Stale and removed Stale labels Jul 17, 2024
@mx-psi mx-psi requested a review from jaronoff97 July 18, 2024 11:22
@mx-psi
Copy link
Member

mx-psi commented Jul 18, 2024

@jaronoff97 Can you take another look?

@jaronoff97
Copy link
Contributor

@mx-psi added another comment, i think other collector maintainers/developers should also look at this as well given it would potentially change what the collector is recommended to emit.

@mx-psi mx-psi requested a review from dmitryax July 18, 2024 14:04
Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

I'm curious how configtelemetry level is meant to be used in the collector. Is the idea that it will be used to conditionally record telemetry throughout the codebase (are there any examples)? Have you considered using existing SDK mechanisms to limit telemetry such as samplers and views?

@atoulme
Copy link
Contributor Author

atoulme commented Jul 19, 2024

I'm curious how configtelemetry level is meant to be used in the collector. Is the idea that it will be used to conditionally record telemetry throughout the codebase (are there any examples)? Have you considered using existing SDK mechanisms to limit telemetry such as samplers and views?

you can look at obsreport to give you an idea of some of the bundled telemetry. We use the Go SDK for metrics and traces.

config/configtelemetry/doc.go Outdated Show resolved Hide resolved
config/configtelemetry/doc.go Outdated Show resolved Hide resolved
@mx-psi mx-psi requested a review from mwear July 26, 2024 11:22
config/configtelemetry/doc.go Outdated Show resolved Hide resolved
config/configtelemetry/doc.go Outdated Show resolved Hide resolved
@djaglowski
Copy link
Member

I opened #10769 with a related feature request for metric levels.

@atoulme atoulme force-pushed the add_guidelines_telemetry branch 2 times, most recently from f046f92 to a9fbae5 Compare August 1, 2024 06:08
@atoulme
Copy link
Contributor Author

atoulme commented Aug 1, 2024

focusing the language on helping component developers make decisions about whether a metric or span should be included at a certain level
That's a good insight and I have added a bit on the intent of the doc at the top. Please help make this better.

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

Thanks @atoulme. This looks good to me.

config/configtelemetry/doc.go Outdated Show resolved Hide resolved
config/configtelemetry/doc.go Outdated Show resolved Hide resolved
config/configtelemetry/doc.go Outdated Show resolved Hide resolved
config/configtelemetry/doc.go Outdated Show resolved Hide resolved
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.

Some suggestions

config/configtelemetry/doc.go Outdated Show resolved Hide resolved
config/configtelemetry/doc.go Outdated Show resolved Hide resolved
config/configtelemetry/doc.go Outdated Show resolved Hide resolved
config/configtelemetry/doc.go Outdated Show resolved Hide resolved
config/configtelemetry/doc.go Outdated Show resolved Hide resolved
config/configtelemetry/doc.go Outdated Show resolved Hide resolved
config/configtelemetry/doc.go Outdated Show resolved Hide resolved
config/configtelemetry/doc.go Outdated Show resolved Hide resolved
config/configtelemetry/doc.go Outdated Show resolved Hide resolved
@mx-psi mx-psi requested a review from dmitryax August 27, 2024 15:42
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.

Looks great, I appreciate that these are now recommendations. I think having basic remain the core telemetry also makes sense to me. Thank you for this.

@codeboten codeboten merged commit 9b4a277 into open-telemetry:main Aug 28, 2024
49 checks passed
@atoulme atoulme deleted the add_guidelines_telemetry branch August 28, 2024 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Describe configtelemetry levels
8 participants