[APMS-20023] Add fallback for hidden URI in JDBC - #6123
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c8a2cbbe4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: f1c3244 | Docs | Datadog PR Page | Give us feedback! |
BenchmarksBenchmark execution time: 2026-07-30 12:21:25 Comparing candidate commit f1c3244 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 48 metrics, 1 unstable metrics.
|
Strech
left a comment
There was a problem hiding this comment.
To be honest changes are not easy to follow (so to say)
| begin | ||
| db.synchronize do |conn| | ||
| conn.get_meta_data.get_url if conn.respond_to?(:get_meta_data) | ||
| end | ||
| rescue => e | ||
| Datadog.logger.debug { "Sequel: unable to resolve JDBC connection metadata (#{e.class})" } | ||
| return nil | ||
| end |
There was a problem hiding this comment.
If we return nil, does it make more senseto rescue in method body instead to return nil? It will be a ruby-style, not AI style then
def method
# ...
url = "..."
# ...
rescue
logger { "..." }
nil
end| if parsed[:host].nil? && parsed[:port].nil? && parsed[:database].nil? | ||
| parsed = jdbc_metadata_from_connection(db) || parsed | ||
| end |
There was a problem hiding this comment.
In this condition, we know for a fact that parsed host/port/database are nil, does it make sense to fallback to parsed here?
| # Sequel's JDBC adapter connects with the URL and ignores separate | ||
| # :host/:port options, unlike native adapters where those options take precedence. | ||
| if !parsed[:host].nil? || !parsed[:port].nil? || !parsed[:database].nil? | ||
| if parsed[:host] || parsed[:port] || parsed[:database] |
There was a problem hiding this comment.
Before the condition was !nil and we used those values in assignments, not we potentially can have host or port as nil value, not sure that I can follow
What does this PR do?
Add fallback for hidden URI in sequel with JDBC. With DataSource/JNDI-managed connection it is possible to have a hidden URI that can still be access through
Motivation:
APMS-20023
Change log entry
None. We do not officially support JRuby anymore and this is specific to JRuby
Additional Notes:
How to test the change?
The CI does not test this because we do not test on JRuby anymore. So the tests are best effort and reproduce what would happen on JRuby, in a Ruby env