feat (sequel): Initial Sequel instrumentation #672

jrgns · 2023-09-24T09:17:42Z

OpenTelemetry Instrumentation for the Sequel Database Toolkit.

This can be used instead of DB specific instrumentations.

arielvalentin · 2023-09-24T15:33:35Z

@jrgns Thank you for your contribution!

I will try to give this a review later this week but I have a few questions for you before we begin the process:

Are you a member of the OTel GitHub Organization?

Are you willing to commit to providing on-going maintenance for this instrumentation? E.g. Review PRs, Fix Vulnerabilities, Maintain Version Compatibility, etc...

jrgns · 2023-09-24T15:41:52Z

Hey @arielvalentin

No, I'm not part of the OTel Github Org

Yes. I'm willing to commit to maintenance etc.

arielvalentin · 2023-09-24T17:53:19Z

@jrgns looks like the generator added the unreleased instrumentation gem as a dependency in all.

Please remove it from all an I'll create a follow up PR to include it once we have a release of this gem published in rubygems.

jrgns · 2023-09-24T20:11:38Z

@arielvalentin check it now?

arielvalentin · 2023-09-25T00:56:13Z

@jrgns I will take a look tomorrow during US business hours

arielvalentin

I have left a few comments for you to review with some requested changes. Please be patient as I am not familiar with the Sequel gem and will need your help digging into some specifics about how extensions work.

Thanks again for this contribution!

instrumentation/sequel/README.md

instrumentation/sequel/lib/opentelemetry/instrumentation/sequel/instrumentation.rb

instrumentation/sequel/lib/sequel/extensions/opentelemetry.rb

instrumentation/sequel/opentelemetry-instrumentation-sequel.gemspec

instrumentation/sequel/lib/sequel/extensions/opentelemetry.rb

kaylareopelle · 2023-10-10T21:07:41Z

@jrgns - My apologies, I forgot to follow up on the CI/release documents in my earlier review.

Could you add your gem to the following files?

.github/workflows/ci-instrumentation-canary.yml
.github/workflows/ci-instrumentation.yml
.toys/.data/releases.yml

This should allow your sequel suite to run as part of the CI checks and will allow the gem to eventually be released. Unfortunately, these file updates aren't included automatically in the instrumentation generator.

instrumentation/sequel/lib/sequel/extensions/opentelemetry.rb

arielvalentin · 2023-10-12T03:01:26Z

instrumentation/sequel/test/opentelemetry/instrumentation/sequel_test.rb

+    exporter.reset
+  end
+
+  describe 'tracing' do


The tests do not seem very specific to me or help me understand as an end user what I should expect to see on a span when I use this.

The tests for traces dataset executes e.g. may continue to pass even though the span does not have the data we expect.

May I ask that you expand on these tests (although it may seem verbose) to include assertions around the spans that were captured?

E.g. name, kind, attributes, if any events where captured etc..

I've made some changes, but will expand it even further.

jrgns · 2023-10-12T20:23:34Z

@kaylareopelle I've added the gem to the CI files.

jrgns · 2023-10-21T06:25:10Z

@arielvalentin anything else you need on this?

instrumentation/sequel/CHANGELOG.md

instrumentation/sequel/test/opentelemetry/instrumentation/sequel_test.rb

jrgns · 2023-10-29T15:23:30Z

@kaylareopelle @arielvalentin changes made.

kaylareopelle · 2023-11-06T18:56:01Z

Thanks, @jrgns! It looks like the sequel tests are failing with JRuby. The CI skips JRuby for Active Record, so perhaps it should be skipped for Sequel too.

On Sequel's homepage, it seems like the gem should be compatible with JRuby, but I think we might to install the JBDC adapter test environment to make it work.

@arielvalentin, I'm not aware of the OTel Ruby policy that drives JRuby testing for instrumentation. Can you provide some guidance here?

arielvalentin · 2023-11-06T19:50:24Z

It's mostly a capacity limitation.

I think we really want to support the JRuby test but none of the existing maintainers use it, so trying to maintain the integration test suite is quite difficult.

If you are up to the challenge of enabling JRuby integration tests then I would welcome your contribution! #4

simi · 2023-11-06T20:27:42Z

tests can be started when sqlite3 is removed from gemspec and added to gemfile

gem 'sqlite3', '~> 1.6.4', platform: :ruby
gem 'jdbc-sqlite3', '< 3.42', platform: :jruby

But tests are still failing due to some difference. I'm looking into.

simi · 2023-11-06T20:54:10Z

instrumentation/sequel/lib/sequel/extensions/opentelemetry.rb

+    def trace_execute(super_method, sequel_method, sql, options, &block)
+      response = nil
+      attributes = {
+        ::OpenTelemetry::SemanticConventions::Trace::DB_NAME => opts[:database],


opts[:database] is nil for jdbc connection like jdbc:sqlite::memory:. @jeremyevans is there any better way in here to get database name? I have checked https://sequel.jeremyevans.net/rdoc/classes/Sequel/Database.html and I wasn't able to find out.

Sequel treats JDBC connection string URIs as opaque, it does not try to parse them. You could try parsing the JDBC URI yourself if you want to find the database name. Note that it isn't unusual to not have a database name even in the non-JDBC case. postgres:///?user=foo will use the default database for user foo on PostgreSQL, but opts[:database] will still be nil in that case.

Than it would make sense to use at least DB_CONNECTION_STRING and fill in jdbc connection string.

https://github.com/open-telemetry/opentelemetry-ruby/blob/d365dfeb3982f09cd62a17c6e67eb3792f56e460/semantic_conventions/lib/opentelemetry/semantic_conventions/trace.rb#L36-L37C7

But it will potentially leak credentials. 🤔

I think the most robust way to handle this is to use Database#database_type to determine the type of database in use, then run database-specific code to get the database name using a query. However, Sequel does not have built-in code to do that, and trying to handle it for all databases that Sequel supports seems challenging.

Considering that approach isn't really feasible, parsing the JDBC URI and returning the path part as the database name is probably the best you can do. At least that gets you behavior similar to other adapters.

I did some experiments, but it is not simple to extract the DB name from JDBC URI in unified way and also it is not possible to redact password in JDBC URI. Considering all this together, would it be acceptable to make this one optional @arielvalentin @kaylareopelle and call compact on attributes?

PS: It would be possible to add DB_JDBC_DRIVER_CLASSNAME, I can open additional PRO to enhance.

It's a bit slower than checking if values are nil before adding them to the hash but using compact! is fine by me.

@arielvalentin no need to call compact, I mean is it ok to continue without DB_NAME if not easily reachable at this point?

Yes that's fine. Instrumentations should do their best to include attributes.

jrgns · 2023-12-02T08:25:39Z

What's the next steps here?

arielvalentin · 2023-12-02T12:58:25Z

Please update your branch and address any merge conflicts.
Fix any failing tests/builds
Re-request a review from folks whose comments you've addressed

github-actions · 2024-01-02T01:46:13Z

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

github-actions · 2024-02-06T01:43:57Z

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

solutus · 2024-10-04T11:06:36Z

Hello, is there any plans to maintain Sequel now?

arielvalentin · 2024-10-04T11:57:48Z

Hello, is there any plans to maintain Sequel now?

Not at the moment.

We don't have anyone who is willing to be a long term maintainer of the instrumentation.

Is that something you'd be interested in helping with?

jrgns force-pushed the feat/sequel-instrumentation branch 3 times, most recently from 72c7f14 to 88afa84 Compare September 24, 2023 14:16

jrgns changed the title ~~feat: Initial Sequel instrumentation~~ feat (sequel): Initial Sequel instrumentation Sep 24, 2023

jrgns marked this pull request as ready for review September 24, 2023 14:23

jrgns requested review from fbogsany, mwear, robertlaurin, dazuma, ericmustin, arielvalentin, ahayworth, plantfansam and robbkidd as code owners September 24, 2023 14:23

jrgns mentioned this pull request Sep 24, 2023

Sequel instrumentation adapter #11

Closed

arielvalentin added instrumentation help wanted Extra attention is needed labels Sep 24, 2023

jrgns force-pushed the feat/sequel-instrumentation branch from 88afa84 to 7eecb44 Compare September 24, 2023 15:47

jrgns force-pushed the feat/sequel-instrumentation branch 3 times, most recently from 702ca41 to 3f9feb2 Compare September 24, 2023 20:04

arielvalentin requested changes Sep 27, 2023

View reviewed changes

jrgns force-pushed the feat/sequel-instrumentation branch 3 times, most recently from cf5cee9 to 9c2fda4 Compare September 27, 2023 20:03

arielvalentin reviewed Oct 12, 2023

View reviewed changes

jrgns force-pushed the feat/sequel-instrumentation branch 2 times, most recently from d2fa547 to 476a85c Compare October 12, 2023 19:39

arielvalentin reviewed Oct 21, 2023

View reviewed changes

instrumentation/sequel/CHANGELOG.md Outdated Show resolved Hide resolved

arielvalentin requested changes Oct 21, 2023

View reviewed changes

instrumentation/sequel/test/opentelemetry/instrumentation/sequel_test.rb Outdated Show resolved Hide resolved

instrumentation/sequel/test/opentelemetry/instrumentation/sequel_test.rb Outdated Show resolved Hide resolved

kaylareopelle requested changes Oct 24, 2023

View reviewed changes

instrumentation/sequel/test/opentelemetry/instrumentation/sequel_test.rb Outdated Show resolved Hide resolved

feat: Initial Sequel instrumentation

f24d7f2

jrgns force-pushed the feat/sequel-instrumentation branch from 476a85c to 88b72f3 Compare October 29, 2023 15:22

jrgns requested a review from simi as a code owner October 29, 2023 15:22

jrgns requested a review from a team October 29, 2023 15:22

jrgns force-pushed the feat/sequel-instrumentation branch from 88b72f3 to f24d7f2 Compare October 29, 2023 15:25

simi reviewed Nov 6, 2023

View reviewed changes

github-actions bot added the stale Marks an issue/PR stale label Jan 2, 2024

jrgns requested a review from a team January 5, 2024 05:44

github-actions bot removed the stale Marks an issue/PR stale label Jan 6, 2024

github-actions bot added the stale Marks an issue/PR stale label Feb 6, 2024

github-actions bot closed this Feb 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat (sequel): Initial Sequel instrumentation #672

feat (sequel): Initial Sequel instrumentation #672

jrgns commented Sep 24, 2023 •

edited

Loading

arielvalentin commented Sep 24, 2023

jrgns commented Sep 24, 2023

arielvalentin commented Sep 24, 2023

jrgns commented Sep 24, 2023

arielvalentin commented Sep 25, 2023

arielvalentin left a comment

kaylareopelle commented Oct 10, 2023

arielvalentin Oct 12, 2023

jrgns Oct 12, 2023

jrgns commented Oct 12, 2023

jrgns commented Oct 21, 2023

jrgns commented Oct 29, 2023

kaylareopelle commented Nov 6, 2023

arielvalentin commented Nov 6, 2023

simi commented Nov 6, 2023

simi Nov 6, 2023

jeremyevans Nov 6, 2023

simi Nov 6, 2023 •

edited

Loading

jeremyevans Nov 6, 2023

simi Nov 7, 2023

arielvalentin Nov 7, 2023

simi Nov 7, 2023

arielvalentin Nov 7, 2023

jrgns commented Dec 2, 2023

arielvalentin commented Dec 2, 2023

github-actions bot commented Jan 2, 2024

github-actions bot commented Feb 6, 2024

solutus commented Oct 4, 2024

arielvalentin commented Oct 4, 2024

feat (sequel): Initial Sequel instrumentation #672

feat (sequel): Initial Sequel instrumentation #672

Conversation

jrgns commented Sep 24, 2023 • edited Loading

arielvalentin commented Sep 24, 2023

jrgns commented Sep 24, 2023

arielvalentin commented Sep 24, 2023

jrgns commented Sep 24, 2023

arielvalentin commented Sep 25, 2023

arielvalentin left a comment

Choose a reason for hiding this comment

kaylareopelle commented Oct 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrgns commented Oct 12, 2023

jrgns commented Oct 21, 2023

jrgns commented Oct 29, 2023

kaylareopelle commented Nov 6, 2023

arielvalentin commented Nov 6, 2023

simi commented Nov 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simi Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrgns commented Dec 2, 2023

arielvalentin commented Dec 2, 2023

github-actions bot commented Jan 2, 2024

github-actions bot commented Feb 6, 2024

solutus commented Oct 4, 2024

arielvalentin commented Oct 4, 2024

jrgns commented Sep 24, 2023 •

edited

Loading

simi Nov 6, 2023 •

edited

Loading