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

fix(tracing): trace hook error out when see nil #10364

Merged
merged 2 commits into from
Mar 9, 2023

Conversation

StarlightIbuki
Copy link
Contributor

@StarlightIbuki StarlightIbuki commented Feb 24, 2023

Summary

"instrument" hooks toip, db_connect and other functions that widely used by core and plugins, and collect data for tracing.

I surprisingly find the hooking function will error out when set_attribute got a nil value. For example, A failing DNS query would emit an error with traceback: "invalid value".

This will definitely change the behavior of the hooked function. toip would have returned (nil, err) if it's not hooked, but instead, it throws an error and the error has nothing to do with the origin error. This could cause more serious issues for other hooked functions.

Luckily, we do not have more cases where set_attributes receives invalid values.

The complete list of hooked functions will be shown offline.

Checklist

  • The Pull Request has tests
  • There's an entry in the CHANGELOG

N/A There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Full changelog

  • Change "error" to "log".

Issue reference

KAG-749

@fffonion
Copy link
Contributor

seems the failing test is related

@StarlightIbuki
Copy link
Contributor Author

seems the failing test is related

It is. But let's wait until the 3.2 released

CHANGELOG.md Outdated Show resolved Hide resolved
Fix KAG-749

Co-authored-by: Wangchong Zhou <wangchong@konghq.com>
@StarlightIbuki
Copy link
Contributor Author

I tried reverting the change and the test still fails for t/01-pdk/08-response/11-exit.t (39, 41, 42, 43).
The master is not red. No idea why it fails at this branch.

@StarlightIbuki StarlightIbuki added the pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) label Mar 8, 2023
@hanshuebner hanshuebner merged commit ff384be into master Mar 9, 2023
@hanshuebner hanshuebner deleted the fix/trace_throw_error branch March 9, 2023 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog core/pdk core/tracing pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants