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

Sequel Support #171

Merged
merged 14 commits into from
Apr 20, 2018
Merged

Sequel Support #171

merged 14 commits into from
Apr 20, 2018

Conversation

ufoot
Copy link
Contributor

@ufoot ufoot commented Aug 8, 2017

Following the work initiated by #163 this PR:

  • includes Adds support for Sequel ruby gem. #163 (which is a very nice starting points, has tests & everything)
  • adds support for execute_ddl, execute_dui and execute_insert (without that last one, we'd get no clue on INSERT calls for instance)
  • completes the test suite

@ufoot ufoot added the integrations Involves tracing integrations label Aug 8, 2017
@ufoot ufoot added this to the 0.8.1 milestone Aug 8, 2017
@ufoot ufoot modified the milestones: 0.8.2, 0.8.1 Aug 9, 2017
@palazzem palazzem modified the milestones: 0.8.2, 0.9.0 Aug 31, 2017
@palazzem palazzem modified the milestone: 0.9.0 Sep 11, 2017
@@ -33,6 +33,8 @@ def self.normalize_vendor(vendor)
'sqlite'
when 'postgresql'
'postgres'
when 'mysql2'
'mysql'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure if you would want to differentiate between mysql and mysql2 adapters. If you do, can probably remove this line, otherwise this should group them together.

end

def sanitize_sql(sql)
regexp = Regexp.new('(\'[\s\S][^\']*\'|\d*\.\d+|\d+|NULL)', Regexp::IGNORECASE)
Copy link
Contributor

@randy-girard randy-girard Dec 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This regexp should be updated and probably extract out into the class:

REGEXP ||= Regexp.new(
            Regexp.union([
              /'(?:[^']|'')*?(?:\\'.*|'(?!'))/,
              /"(?:[^"]|"")*?(?:\\".*|"(?!"))/,
              /\b-?(?:[0-9]+\.)?[0-9]+([eE][+-]?[0-9]+)?\b/,
              /\b(?:true|false|null)\b/i,
              /0x[0-9a-fA-F]+/,
              /(?:#|--).*?(?=\r|\n|$)/i,
              /\/\*(?:[^\/]|\/[^*])*?(?:\*\/|\/\*.*)/
            ])
          )

Replace this following line to sql.to_s.gsub(REGEXP, '?'). This should help speed it up some as well as remove extraneous ?.

@randy-girard
Copy link
Contributor

Added a few comments. Otherwise looks good!

@thiago-sydow
Copy link

Hello! Is there something missing/blocked in this PR? I was going to start using this gem but just now realized the lack of support for Sequel

@palazzem
Copy link
Contributor

palazzem commented Jan 8, 2018

Hello @thiago-sydow ! thanks for reaching us! At the moment we're planning to ship a new release with a lot of changes. Unfortunately Sequel support will not be added in this release but in the next one. I can ping you again once the review (and the refactoring with the new config system) is in our weekly cycle.

Sorry for the delay!

@thiago-sydow
Copy link

Hi @palazzem thank you for replying! I understand, and I would love to get notified when Sequel is worked/merged.

I checked the new config system to be released and seem far better then the current one, nice job!

@palazzem
Copy link
Contributor

@thiago-sydow thank you very much! Will ping everyone in this thread once we move this PR review from the backlog to the current cycle of work.

@delner delner added the feature Involves a product feature label Feb 5, 2018
@delner delner mentioned this pull request Apr 5, 2018
@palazzem palazzem added this to the 0.13.0 milestone Apr 16, 2018
@delner delner force-pushed the christian/add-sequel-support branch from 0c2b372 to 15495b5 Compare April 18, 2018 16:55
@delner delner changed the base branch from master to 0.13-dev April 18, 2018 16:56
@delner delner force-pushed the christian/add-sequel-support branch from 53d11af to 5320f62 Compare April 18, 2018 18:09
@delner
Copy link
Contributor

delner commented Apr 18, 2018

Updated this pull request to 0.13-dev:

  • Squashed previous commits
  • Fixed merge conflicts
  • Changed to not use Monkey
  • Converted tests from Minitest to RSpec

Still need to:

  • Update use of Pin in favor of Datadog.configure
  • Update documentation
  • Change how it quantizes SQL, and restructure the span
  • Review how we patch Sequel (is there a better way than re-writes?)

@delner delner force-pushed the christian/add-sequel-support branch 2 times, most recently from 7d0b52c to fd425e5 Compare April 19, 2018 17:49
@delner
Copy link
Contributor

delner commented Apr 19, 2018

In addition to the above, I also:

  • Extracted #normalize_vendor from ActiveRecord into Datadog::Utils::Database so this integration could use it without depending on the ActiveRecord integration.
  • Changed the default from sequel to whatever the adapter name is. If you set a service name via #use or directly against the database object, that one is used instead.

@delner delner requested a review from palazzem April 19, 2018 19:26
@delner delner self-assigned this Apr 19, 2018
@delner delner force-pushed the christian/add-sequel-support branch from cb99057 to c13b8d8 Compare April 20, 2018 17:09
@palazzem
Copy link
Contributor

Hey @thiago-sydow ! we've finished the rebase and all the work around Sequel! Actually the 0.12 is in release candidate but Sequel will be released in the 0.13 (usually we do a minor release every 2-3 weeks). Before that time, we'll ship probably a beta release so all these new integrations can be tested beforehand.

Sounds good?

@delner delner merged commit c51f3ac into 0.13-dev Apr 20, 2018
@delner delner deleted the christian/add-sequel-support branch April 20, 2018 17:27
@thiago-sydow
Copy link

Hi @palazzem thank you for notifying me, yes that sounds great!

@delner
Copy link
Contributor

delner commented Apr 23, 2018

Also, @palin just letting you know this one is merged!

@twe4ked
Copy link

twe4ked commented Apr 24, 2018

I'm having trouble getting this to work. I've tried a few different versions of the gems. I'm trying to match what I've installed locally using the appraisals gem. I get get the ddtrace specs to pass but I can't get the example to run.

Example gist: https://gist.github.com/twe4ked/590db5a686104770a53a78b657eacc3d

Thanks.

Edit: The Database#initialize method never seems to get called.

@delner
Copy link
Contributor

delner commented Apr 24, 2018

@twe4ked Hmmm, if initialize isn't being called, then maybe it isn't patching. I'll take a look at this, too. Thanks for the Gist.

@twe4ked
Copy link

twe4ked commented Apr 24, 2018

No worries. It seems to prepend the InstanceMethods ok. I've also confirmed when I run the ddtrace specs that the initialize method is called so that definitely seems like the right path.

@delner
Copy link
Contributor

delner commented Apr 24, 2018

Cool, let me know if you find anything else.

@delner
Copy link
Contributor

delner commented Apr 24, 2018

Oh I think I know why: your Sequel::Database is created before Datadog.configure runs. If you configure before creating the database, it should work.

This is because it patches #initialize, which adds a Pin configuration to the Database object. Though #initialize does get patched, it only happens after the database was created, thus the database has no tracer configuration.

I suppose we could probably add some kind of default pin to the Sequel::Database constant, so databases will at least get some default tracing if they were instantiated too early, as opposed to nothing.

@twe4ked I'll open a bug fix PR for this. Thanks for reporting!

@twe4ked
Copy link

twe4ked commented Apr 24, 2018

That's it! Thanks for your help.

@palin
Copy link

palin commented Apr 25, 2018

Amazing! Thanks!

@delner
Copy link
Contributor

delner commented Apr 25, 2018

@twe4ked Pull request for the bug fix open here: #408. Will hopefully merge that to 0.13-dev soon.

@twe4ked
Copy link

twe4ked commented May 7, 2018

Any update on getting a beta release out soon? I'm guessing it might be a while since 0.12.0 is still in RC phase. Cheers.

@palazzem
Copy link
Contributor

palazzem commented May 8, 2018

Hello @twe4ked! yes we're going to ship the 0.12.0 this week, so that the first 0.13.beta1 can be out next one. Thanks for your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Involves a product feature integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants