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

Support Trilogy gem #3274

Merged
merged 5 commits into from
Jan 22, 2024
Merged

Support Trilogy gem #3274

merged 5 commits into from
Jan 22, 2024

Conversation

y-yagi
Copy link
Contributor

@y-yagi y-yagi commented Nov 22, 2023

What does this PR do?

This PR adds the support of Trilogy gem.

Motivation:

The Trilogy gem is MySQL-compatible database adapter and developed by GitHub.
https://github.blog/2022-08-25-introducing-trilogy-a-new-database-adapter-for-ruby-on-rails/

Rails has supported this since v7.1.
https://guides.rubyonrails.org/7_1_release_notes.html#introduce-adapter-for-trilogy

GitHub also has introduced the Active Record database adapter for older Rails. So users can use this gem with Rails before v7.1.
https://github.com/trilogy-libraries/activerecord-trilogy-adapter

I would like to add support for this gem in ddtrace.

Additional Notes:

Trilogy gem doesn't set required_ruby_version, so I'm not sure whether it works on older Ruby or not. But Ruby 2.x series are already EOL. So I set Ruby versions for test only 3.x series.

How to test the change?

I added test files for this change.

For Datadog employees:

  • If this PR touches code that signs or publishes builds or packages, or handles
    credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

@github-actions github-actions bot added integrations Involves tracing integrations tracing labels Nov 22, 2023
@y-yagi y-yagi force-pushed the support-trilogy branch 3 times, most recently from 86373e2 to 2f824ab Compare November 22, 2023 05:01
@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f3358c0) 98.23% compared to head (a4b4dff) 98.23%.
Report is 18 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #3274    +/-   ##
========================================
  Coverage   98.23%   98.23%            
========================================
  Files        1253     1261     +8     
  Lines       72690    72948   +258     
  Branches     3415     3422     +7     
========================================
+ Hits        71405    71663   +258     
  Misses       1285     1285            

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@y-yagi y-yagi marked this pull request as ready for review November 22, 2023 07:54
@y-yagi y-yagi requested review from a team as code owners November 22, 2023 07:54
Copy link

@estherk15 estherk15 left a comment

Choose a reason for hiding this comment

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

Looks good for documentation, left some very optional feedback

@@ -2023,6 +2023,29 @@ end
LogJob.perform_async('login')
```

### Trilogy

The trilogy integration traces any SQL command sent through `trilogy` gem.

Choose a reason for hiding this comment

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

Suggested change
The trilogy integration traces any SQL command sent through `trilogy` gem.
The trilogy integration traces any SQL command sent through the `trilogy` gem.

`options` are the following keyword arguments:

| Key | Env Var | Description | Default |
|-----------------------|--------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|--------------|

Choose a reason for hiding this comment

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

Suggested change
|-----------------------|--------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|--------------|
|-----------------------|---------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-----------|


| Key | Env Var | Description | Default |
|-----------------------|--------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|--------------|
| `service_name` | `DD_TRACE_TRILOGY_SERVICE_NAME` | Name of application running the `trilogy` instrumentation. May be overridden by `global_default_service_name`. [See *Additional Configuration* for more details](#additional-configuration) | `trilogy` |

Choose a reason for hiding this comment

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

Suggested change
| `service_name` | `DD_TRACE_TRILOGY_SERVICE_NAME` | Name of application running the `trilogy` instrumentation. May be overridden by `global_default_service_name`. [See *Additional Configuration* for more details](#additional-configuration) | `trilogy` |
| `service_name` | `DD_TRACE_TRILOGY_SERVICE_NAME` | Name of application running the `trilogy` instrumentation. May be overridden by `global_default_service_name`. [See *Additional Configuration* for more details](#additional-configuration) | `trilogy` |

| Key | Env Var | Description | Default |
|-----------------------|--------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|--------------|
| `service_name` | `DD_TRACE_TRILOGY_SERVICE_NAME` | Name of application running the `trilogy` instrumentation. May be overridden by `global_default_service_name`. [See *Additional Configuration* for more details](#additional-configuration) | `trilogy` |
| `peer_service` | `DD_TRACE_TRILOGY_PEER_SERVICE` | Name of external service the application connects to | `nil` |

Choose a reason for hiding this comment

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

Suggested change
| `peer_service` | `DD_TRACE_TRILOGY_PEER_SERVICE` | Name of external service the application connects to | `nil` |
| `peer_service` | `DD_TRACE_TRILOGY_PEER_SERVICE` | Name of external service the application connects to | `nil` |

@marcotc
Copy link
Member

marcotc commented Nov 22, 2023

Thank you so much, @y-yagi! We'll likely give your changes a thorough review after the U.S. thanksgiving holiday.

@y-yagi
Copy link
Contributor Author

y-yagi commented Nov 23, 2023

@estherk15
Thanks for your review! I fixed all your pointed!

Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

Awesome work, @y-yagi!

One last thing, please take a look at the type checking Action that I just allowed to run, and try to address the typing issues if possible: https://github.com/DataDog/dd-trace-rb/actions/runs/6975162485/job/19386758433?pr=3274
Because third-party integrations heavily integrate with external gems, it might not be possible to fully type it, which is totally fine (and you add a file to the ignore list). The rule of thumb is: if it's typing gets crazy, feel free to ignore the file.

@y-yagi
Copy link
Contributor Author

y-yagi commented Dec 7, 2023

@marcotc
Thanks for your review! I have added signature files for Trilogy integration(Those are almost just generated files. Sorry!)

@TonyCTHsu TonyCTHsu added this to the 1.19.0 milestone Dec 11, 2023
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

Thank you so much, @y-yagi! 🙇

@ekump ekump modified the milestones: 1.19.0, 1.20.0 Jan 10, 2024
@ekump ekump merged commit 57976d5 into DataDog:master Jan 22, 2024
54 of 55 checks passed
@TonyCTHsu TonyCTHsu mentioned this pull request Feb 5, 2024
@TonyCTHsu TonyCTHsu added the community Was opened by a community member label Apr 2, 2024
TonyCTHsu pushed a commit that referenced this pull request Apr 19, 2024
Support `trilogy` gem

---------

Co-authored-by: y-yagi <y-yagi@users.noreply.github.com>
Co-authored-by: Edmund Kump <edmund.kump@datadoghq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Was opened by a community member integrations Involves tracing integrations tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants