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

Validate add/record operations for instruments #2394

Merged
merged 2 commits into from
Jan 20, 2022
Merged

Conversation

lzchen
Copy link
Contributor

@lzchen lzchen commented Jan 20, 2022

Fixes #2393

@ocelotl
I know some of these MAY get addressed in an abstract manner with your error handling pr but just adding these checks for now for completion and until we decide to move forward with a generic error handling method. Other language implementations have this in the SDK as well.

@lzchen lzchen requested a review from a team January 20, 2022 02:41
@lzchen lzchen added Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary Skip Changelog PRs that do not require a CHANGELOG.md entry labels Jan 20, 2022
@ocelotl
Copy link
Contributor

ocelotl commented Jan 20, 2022

Fixes #2393

@ocelotl I know some of these MAY get addressed in an abstract manner with your error handling pr but just adding these checks for now for completion and until we decide to move forward with a generic error handling method. Other language implementations have this in the SDK as well.

Minor nits only, approved 😎

Just to be clear, the error handling mechanism will not address any of these checks in the sense that it will not implement the checks themselves. What would happen if the error handling mechanism was in place is that these checks would be still implemented somewhere (in the SDK or in the API (the latter would make it much easier to handle errors)), but they would raise an exception instead of logging a warning. What the error handling mechanism would do is to turn that exception into a warning (not to be confused with a logging warning) and handle it appropriately. ✌️

@lzchen lzchen merged commit 6bdc5fd into open-telemetry:main Jan 20, 2022
@lzchen lzchen deleted the warn branch January 20, 2022 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate operations for instruments
3 participants