-
Notifications
You must be signed in to change notification settings - Fork 368
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
Support Trilogy
gem
#3274
Conversation
86373e2
to
2f824ab
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
There was a problem hiding this 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
docs/GettingStarted.md
Outdated
@@ -2023,6 +2023,29 @@ end | |||
LogJob.perform_async('login') | |||
``` | |||
|
|||
### Trilogy | |||
|
|||
The trilogy integration traces any SQL command sent through `trilogy` gem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trilogy integration traces any SQL command sent through `trilogy` gem. | |
The trilogy integration traces any SQL command sent through the `trilogy` gem. |
docs/GettingStarted.md
Outdated
`options` are the following keyword arguments: | ||
|
||
| Key | Env Var | Description | Default | | ||
|-----------------------|--------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|--------------| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|-----------------------|--------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|--------------| | |
|-----------------------|---------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-----------| |
docs/GettingStarted.md
Outdated
|
||
| 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` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| `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` | |
docs/GettingStarted.md
Outdated
| 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` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| `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` | |
Thank you so much, @y-yagi! We'll likely give your changes a thorough review after the U.S. thanksgiving holiday. |
2f824ab
to
1f4afa4
Compare
@estherk15 |
There was a problem hiding this 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.
f1dba42
to
62d1a2f
Compare
@marcotc |
There was a problem hiding this 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! 🙇
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 setrequired_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:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance
.Unsure? Have a question? Request a review!