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

[PROF-5808] Add ENFORCE_TYPE as user-friendly alternative to Check_Type #2156

Merged
merged 2 commits into from
Jul 18, 2022

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Jul 14, 2022

Motivation:

While investigating #2151, I realized that Check_Type's error message: TypeError: wrong argument type nil (expected String) is very hard to debug in functions that do many Check_Type verifications (such as _native_do_export).

What does this PR do?:

To fix this, I've created a new ENFORCE_TYPE helper which behaves similarly, but generates error messages such as:

TypeError: wrong argument nil for 'pprof_file_name' (expected a RUBY_T_STRING) at ../../../../ext/ddtrace_profiling_native_extension/http_transport.c:280:in `_native_do_export'

Hopefully this message will help us get to the bottom of the issue.

I've modified every other use of Check_Type in the profiling native extension to use this new helper, in case we need to debug similar issues in the future.

Additional notes:

This PR is on top of #2152 to facilitate testing both together, but is otherwise independent.

How to test the change?:

Existing tests should already cover this. To trigger the issue manually, use for instance:

[3] pry(main)> Datadog::Profiling::HttpTransport._native_do_export(nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil)
TypeError: wrong argument nil for 'upload_timeout_milliseconds' (expected a RUBY_T_FIXNUM) at ../../../../ext/ddtrace_profiling_native_extension/http_transport.c:275:in `_native_do_export'
from (pry):3:in `_native_do_export'

Issue #2151

ivoanjo added 2 commits July 14, 2022 10:14
…_Type`

While investigating #2151, I realized that `Check_Type`'s error message:
`TypeError: wrong argument type nil (expected String)` is very hard to
debug in functions that do many `Check_Type` verifications
(such as `_native_do_export`).

To fix this, I've created a new `ENFORCE_TYPE` helper which behaves
similarly, but generates error messages such as:

```
TypeError: wrong argument nil for 'pprof_file_name' (expected a RUBY_T_STRING)
at ../../../../ext/ddtrace_profiling_native_extension/http_transport.c:280:in `_native_do_export'
```

Hopefully this message will help us get to the bottom of the issue.

I've modified every other use of `Check_Type` in the profiling
native extension to use this new helper, in case we need to debug
similar issues in the future.

Issue #2151
Base automatically changed from ivoanjo/ensure-all-tags-are-strings to master July 18, 2022 08:17
@ivoanjo ivoanjo merged commit 86186a5 into master Jul 18, 2022
@ivoanjo ivoanjo deleted the ivoanjo/prof-5808-improve-error-messages branch July 18, 2022 08:17
@github-actions github-actions bot added this to the 1.3.0 milestone Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants