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

Change in behavior in 2.0 for the Rails activerecord integration #4179

Open
alexevanczuk opened this issue Dec 2, 2024 · 6 comments
Open
Assignees
Labels
bug Involves a bug community Was opened by a community member

Comments

@alexevanczuk
Copy link
Contributor

The rails instrumentation sets up an activerecord integration here: https://github.com/DataDog/dd-trace-rb/blob/master/lib/datadog/tracing/contrib/rails/framework.rb

This sets up a service called postgres via the activerecord configuration (lib/datadog/tracing/contrib/active_record/configuration/settings.rb) which calls Utils.adapter_name which calls Contrib::Utils::Database.normalize_vendor(connection_config[:adapter]) which evaluates to postgres.

In our postgres service, we're now seeing new errors come in after the 2.0 upgrade. We typically turn off error propagation for our instrumentations because it results in double signal. That is – if an error propagates up and causes an application bug, it results in a bug in an outer span beyond the instrumentation. Normally we just set the on_error (or error_handler prior to the 2.0 upgrade) as a no-op to not attach these errors to these instrumentation spans. However, we didn't need this for the rails instrumentation before, and now after 2.0 there doesn't appear to be a configuration option in lib/datadog/tracing/contrib/rails/configuration/settings.rb that allows us to no-op these postgres errors.

Please let me know your recommendation for disabling these errors again! Thank you!

@alexevanczuk alexevanczuk added bug Involves a bug community Was opened by a community member labels Dec 2, 2024
@alexevanczuk
Copy link
Contributor Author

Hey @marcotc wanted to bump this one!

@marcotc
Copy link
Member

marcotc commented Dec 9, 2024

Hey @alexevanczuk,

I'm not sure I fully understand: what span exactly are you trying to set up an on_error on?

@marcotc marcotc self-assigned this Dec 9, 2024
@alexevanczuk
Copy link
Contributor Author

Hey @marcotc there are spans on the postgres service – I don't want errors to ever be attached to spans created by this integration.

@alexevanczuk
Copy link
Contributor Author

alexevanczuk commented Dec 19, 2024

Hey @marcotc wanted to bump this.

If it helps, here's the diff of the Gemfile.lock that resulted in the change in behavior:

diff --git a/Gemfile.lock b/Gemfile.lock
index dfc055f0b8..51ba37fbb4 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -226,16 +226,13 @@ GEM
-    datadog-ci (0.8.3)
+    datadog (2.7.0)
+      datadog-ruby_core_source (~> 3.3)
+      libdatadog (~> 14.1.0.1.0)
+      libddwaf (~> 1.15.0.0.0)
       msgpack
+    datadog-ruby_core_source (3.3.6)
     date (3.4.0)
-    ddtrace (1.23.3)
-      datadog-ci (~> 0.8.1)
-      debase-ruby_core_source (= 3.3.1)
-      libdatadog (~> 7.0.0.1.0)
-      libddwaf (~> 1.14.0.0.0)
-      msgpack
-    debase-ruby_core_source (3.3.1)
-    libdatadog (7.0.0.1.0)
-    libdatadog (7.0.0.1.0-x86_64-linux)
-    libddwaf (1.14.0.0.0-arm64-darwin)
+    libdatadog (14.1.0.1.0)
+    libdatadog (14.1.0.1.0-x86_64-linux)
+    libddwaf (1.15.0.0.0-arm64-darwin)
       ffi (~> 1.0)
-    libddwaf (1.14.0.0.0-x86_64-darwin)
+    libddwaf (1.15.0.0.0-x86_64-darwin)
       ffi (~> 1.0)
-    libddwaf (1.14.0.0.0-x86_64-linux)
+    libddwaf (1.15.0.0.0-x86_64-linux)
       ffi (~> 1.0)
-  ddtrace
+  datadog

I turned on DD_TRACE_DEBUG locally and confirmed the change in behavior.

Before

 Name: postgres.query
Span ID: 3750229346333265587
Parent ID: 0
Trace ID: 137428280093468097146113802550593583512
Type: sql
Service: postgres
Resource: SELECT..... 'redacted'
Error: 0
Start: 1734588759072848896
End: 1734588759075155968
Duration: 0.002306999987922609
Tags: [
   git.commit.sha => 'redacted',
   git.repository_url => 'redacted',
   env => development,
   version => 'redacted',
   component => active_record,
   operation => sql,
   _dd.base_service =>'redacted',
   active_record.db.vendor => postgres,
   active_record.db.name => 'redacted',
An error occurred when inspecting the object: #<ActiveRecord::StatementInvalid:"PG::UndefinedColumn: ERROR:

After

 Name: postgres.query
Span ID: 24744239621517181
Parent ID: 0
Trace ID: 137428293007658586970028717304565720437
Type: sql
Service: postgres
Resource: SELECT redacted
Error: 1
Start: 1734588922773380096
End: 1734588922776274944
Duration: 0.002892000018619001
Tags: [
   git.commit.sha => 'redacted',
   git.repository_url => 'redacted',
   env => development,
   version => 'redacted',
   component => active_record,
   operation => sql,
   _dd.base_service => monorail,
   active_record.db.vendor => postgres,
   db.instance => 'redacted',
   active_record.db.name => 'redacted',
   out.host => localhost,
   error.type => ActiveRecord::StatementInvalid,
   error.message => PG::UndefinedColumn: ERROR:  column "'redacted'" does not exist
LINE 1: ...''redacted'

Notice that with the same configuration it's now attaching an error to the span. Can you help me figure out where this behavior is? I tried messing with it for a while, but was having a lot of trouble tracing through the DD code and determining why the error was being set now.

@alexevanczuk
Copy link
Contributor Author

Okay, it looks like we're not passing span options to the active support notification and always setting the span error here:

From: /Users/alexevanczuk/workspace/dd-trace-rb/lib/datadog/tracing/contrib/active_support/notifications/event.rb:82 Datadog::Tracing::Contrib::ActiveSupport::Notifications::Event::ClassMethods#record_exception:

    80: def record_exception(span, payload)
    81:   pry
 => 82:   if payload[:exception_object]
    83:     span.set_error(payload[:exception_object])
    84:   elsif payload[:exception]
    85:     # Fallback for ActiveSupport < 5.0
    86:     span.set_error(payload[:exception])
    87:   end
    88: end

I'm still unclear why there was no error before but now there is one though.

@alexevanczuk
Copy link
Contributor Author

@TonyCTHsu Wondering if you might be able to help with this! This results in really confusing error messages – such as related to incorrect queries in a rails console – that I'd like to turn off immediately and go back to the previous behavior in 1.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug community Was opened by a community member
Projects
None yet
Development

No branches or pull requests

2 participants