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

[docs-metrics] Remove predicate-based View examples #5554

Merged

Conversation

reyang
Copy link
Member

@reyang reyang commented Apr 19, 2024

Most users got it wrong, and the error is not detected/reported at SDK initialization time (the exceptions simply got swallowed) which makes it a big pit of failures.

@reyang reyang requested a review from a team as a code owner April 19, 2024 03:15
Copy link

codecov bot commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.55%. Comparing base (6250307) to head (c1214c4).
Report is 187 commits behind head on main.

❗ Current head c1214c4 differs from pull request most recent head b27340e. Consider uploading reports for the commit b27340e to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5554      +/-   ##
==========================================
+ Coverage   83.38%   85.55%   +2.17%     
==========================================
  Files         297      289       -8     
  Lines       12531    12493      -38     
==========================================
+ Hits        10449    10689     +240     
+ Misses       2082     1804     -278     
Flag Coverage Δ
unittests ?
unittests-Solution-Experimental 85.57% <ø> (?)
unittests-Solution-Stable 85.47% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 77 files with indirect coverage changes

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

LGTM.

@alanwest Please review. We have discussed the potential pitfalls from View long ago!

@CodeBlanch
Copy link
Member

Most users got it wrong

Will having less/no documentation help with that? If we don't want to show this most places I'm fine with that. But IMO we should have an advanced doc/section that at least calls out the potential pitfalls for anyone who chooses to use it.

@CodeBlanch CodeBlanch changed the title Remove predicate-based View examples [docs-metrics] Remove predicate-based View examples Apr 19, 2024
@reyang
Copy link
Member Author

reyang commented Apr 19, 2024

Most users got it wrong

Will having less/no documentation help with that? If we don't want to show this most places I'm fine with that. But IMO we should have an advanced doc/section that at least calls out the potential pitfalls for anyone who chooses to use it.

I think that's the right direction. Eventually we might want to deprecate this API, and if there are good number of users who need certain feature, another better API might be introduced which doesn't have this callback pattern.

@CodeBlanch CodeBlanch merged commit f76d21a into open-telemetry:main Apr 19, 2024
31 checks passed
@reyang reyang deleted the reyang/remove-view-predicate-example branch April 19, 2024 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants