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

1.2.0 Profiler regression: "Unable to report profile. Cause: wrong argument type nil (expected String)" #2151

Closed
ivoanjo opened this issue Jul 13, 2022 · 7 comments
Assignees
Labels
bug Involves a bug profiling Involves Datadog profiling

Comments

@ivoanjo
Copy link
Member

ivoanjo commented Jul 13, 2022

Originally posted by @seuros in #2067 (comment)

@ivoanjo : now with the update i get Unable to report profile. Cause: wrong argument type nil (expected String) Location: /var/cache/bundle/ruby/3.0.0/gems/ddtrace-1.2.0/lib/datadog/profiling/http_transport.rb:115:in _native_do_export'

same settings worked in 1.0.0

Looks like in some situations, a value that was assumed to always be a String is coming in as nil. I'm investigating where that nil may be coming from.

@ivoanjo
Copy link
Member Author

ivoanjo commented Jul 13, 2022

@seuros in 1.2.0 I've added a fallback in case there was an issue with the new transport codepath (which unfortunately there is).

The issue should go away if you run with the DD_PROFILING_LEGACY_TRANSPORT_ENABLED environment variable set to true (or alternatively via the configuration block, the config.profiling.advanced.legacy_transport_enabled = true setting). (This workaround is no longer needed)

I suspect this issue may be due to application-wide tags that are set to nil. If possible, could you share the output of Datadog.configuration.tags in your system?

ivoanjo added a commit that referenced this issue Jul 13, 2022
A customer reported having their profiles fail with the following
issue:

> Unable to report profile. Cause: wrong argument type nil (expected String) Location: /var/cache/bundle/ruby/3.0.0/gems/ddtrace-1.2.0/lib/datadog/profiling/http_transport.rb:115:in _native_do_export'

Unfortunately there's a bunch of data passed in to that function so
just from that information it's not possible to be 100% sure which one
was `nil`, but while doing a second pass at the code I noticed one way
to trigger this would be to have tags that are `nil`.

Thus here's a quick fix to ensure that the tags passed on to
profiling are always strings.

Issue #2151
@ivoanjo ivoanjo self-assigned this Jul 13, 2022
@ivoanjo ivoanjo added bug Involves a bug profiling Involves Datadog profiling labels Jul 13, 2022
@seuros
Copy link
Contributor

seuros commented Jul 13, 2022

If possible, could you share the output of Datadog.configuration.tags in your system?

irb(main):001:0> Datadog.configuration.tags
=> {"env"=>"production", "service"=>"railsapp" }

@ivoanjo
Copy link
Member Author

ivoanjo commented Jul 13, 2022

Interesting; so that does not seem to be the source of the issue. I'll cook up a PR to improve the error messages in this case so we can debug this.

In the meanwhile, the DD_PROFILING_LEGACY_TRANSPORT_ENABLED flag I mentioned above should allow you to remain on 1.2.0 (it tells dd-trace-rb to adopt the 1.0.0 behavior for reporting profiles). (This flag is no longer needed)

ivoanjo added a commit that referenced this issue Jul 14, 2022
…_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
@ivoanjo
Copy link
Member Author

ivoanjo commented Jul 14, 2022

I'm still not quite sure where the nil is coming from. To get to the bottom of this, I've opened #2156 which improves the type check to include exactly where in the profiling native extension the issue is getting triggered.

If you're open to it, you can test that PR directly by downloading the gemfile from here (you can install it with gem install <the gem>). Alternatively, you can also add the following to your Gemfile:

source 'https://s3.amazonaws.com/gems.datadoghq.com/prerelease-v2' do
  gem 'ddtrace', '= 1.2.0.ivoanjo.prof.5808.improve.error.messages.246966'
end

@Thomascountz
Copy link

@ivoanjo, @nathaliagiusti and I ran into a similar issue on v1.2.0, except the error complained about a false value instead of nil.

ERROR -- ddtrace: [ddtrace] (/usr/bundle/ruby/2.7.0/gems/ddtrace-1.2.0/lib/datadog/profiling/scheduler.rb:125:in `rescue in flush_events') Unable to report profile. Cause: wrong argument type false (expected String) Location: /usr/bundle/ruby/2.7.0/gems/ddtrace-1.2.0/lib/datadog/profiling/http_transport.rb:115:in `_native_do_export'

In our case, our configuration included these tags:

Datadog.configuration.tags
#=> { my_tag: false }

We tested with your 1.2.0.ivoanjo.prof.5808.improve.error.messages.246966 branch and the error message disappeared so we couldn't check out the additional error information provided by ENFORCE_TYPE.

All that said, after taking a look at #2152, we changed our tag to a String instead of a Boolean:

Datadog.configuration.tags
#=> { my_tag: 'false' }

And we no longer experienced the wrong argument type false (expected String) error. 🎉

@seuros
Copy link
Contributor

seuros commented Aug 4, 2022

That true, the tags hash need to have a string in all it value. I had a version as float.

@ivoanjo
Copy link
Member Author

ivoanjo commented Aug 4, 2022

Thanks @Thomascountz and @seuros for the feedback!

Indeed as part of #2152 we now call to_s on every tag so even non-string values should be included correctly.

I'm going to go ahead and close this ticket as fixed by #2152.

A new release of dd-trace-rb that includes that fix should be out within the next hours/days :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug profiling Involves Datadog profiling
Projects
None yet
Development

No branches or pull requests

3 participants