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

make sure otel.Handle is called only if err != nil #206

Merged
merged 3 commits into from
May 21, 2023

Conversation

auvn
Copy link
Contributor

@auvn auvn commented May 19, 2023

If I run my application with otelsql for a sql storage. I have the following in my logs:

2023/05/19 11:58:25
2023/05/19 11:58:25
2023/05/19 11:58:25
2023/05/19 11:58:25
2023/05/19 11:58:25
2023/05/19 11:58:25
2023/05/19 11:58:25

Otelsql uses otel.Handle function regardless of error value.
I suppose nil errors shouldn't go to logs at all.

I'm suggesting this PR in order to handle nil errors silently.

@auvn
Copy link
Contributor Author

auvn commented May 19, 2023

@nhatthm your feedback is appreciated :)

@auvn auvn force-pushed the error-handler-update branch 2 times, most recently from 98ddd8f to 4de7f3a Compare May 19, 2023 20:12
errors.go Outdated Show resolved Hide resolved
@nhatthm
Copy link
Owner

nhatthm commented May 20, 2023

@nhatthm your feedback is appreciated :)

Hey, thanks for the PR. I left only a small comment on the naming.

@nhatthm nhatthm added the bug Something isn't working label May 20, 2023
@auvn auvn force-pushed the error-handler-update branch 2 times, most recently from a584ef4 to ae35d98 Compare May 21, 2023 07:54
@codecov
Copy link

codecov bot commented May 21, 2023

Codecov Report

Merging #206 (2ab096b) into master (ea0ea8d) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #206   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        22    +1     
  Lines         1208      1212    +4     
=========================================
+ Hits          1208      1212    +4     
Flag Coverage Δ
unittests-Linux-X64 100.00% <100.00%> (ø)
unittests-Windows-X64 100.00% <100.00%> (ø)
unittests-macOS-X64 100.00% <100.00%> (ø)

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

Impacted Files Coverage Δ
driver.go 100.00% <100.00%> (ø)
errors.go 100.00% <100.00%> (ø)
stats.go 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@nhatthm nhatthm enabled auto-merge (squash) May 21, 2023 09:43
@nhatthm nhatthm merged commit 4e4443b into nhatthm:master May 21, 2023
24 checks passed
@auvn auvn deleted the error-handler-update branch May 21, 2023 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants