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

Adds Amazon Timestream ActiveRecord Adapter #1872

Merged
merged 1 commit into from
Mar 10, 2023
Merged

Conversation

wagner
Copy link
Contributor

@wagner wagner commented Mar 10, 2023

Overview

This pull request adds the Amazon Timestream ActiveRecord Adapter to the list of known AR adapters. I don't know which are the requirements for relevance here, but if this gem doesn't meet them, let me know.

Submitter Checklist:

  • [ ] Include a link to the related GitHub issue, if applicable N/A
  • [ ] Include a security review link, if applicable N/A

Reviewer Checklist

  • Perform code review
  • Add performance label
  • Perform appropriate level of performance testing
  • Confirm all checks passed
  • Add version label prior to acceptance

@CLAassistant
Copy link

CLAassistant commented Mar 10, 2023

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the community To tag external issues and PRs submitted by the community label Mar 10, 2023
@fallwith
Copy link
Contributor

Hi @wagner, thanks very much for this contribution!

Typically a new adapter value gets added for one of two reasons: a) in order to tell the agent how to treat the adapter when it comes to something such as SQL obfuscation (see the DIALECT_COMPONENTS and CLEANUP_REGEX constants in lib/new_relic/agent/database/obfuscation_helpers.rb), and b) for display purposes.

If this PR helps your use case, then we are quite happy to bring it in. If there is anything else you'd like to see done to better offer support for the Timestream adapter, please let us know.

Also, do you happen to know if that adapter is still actively maintained or if there are other Timestream adapters available? I see that the adapter is pinned to Rails <= 6 and not compatible with Rails 7, so I'm curious about what users will do when they want to use Rails 7+.

@fallwith fallwith merged commit 17257ac into newrelic:dev Mar 10, 2023
@fallwith
Copy link
Contributor

This change should be released as part of the next v9.x.x agent release. Thanks again!

@wagner
Copy link
Contributor Author

wagner commented Mar 15, 2023

Also, do you happen to know if that adapter is still actively maintained or if there are other Timestream adapters available?

No, that's the only one, as far as I know.

I see that the adapter is pinned to Rails <= 6 and not compatible with Rails 7, so I'm curious about what users will do when they want to use Rails 7+.

I'm not the author, but it looks like Rails 7 support was added on the main branch, although not officially released.

Thanks for merging!

@wagner wagner deleted the patch-1 branch March 15, 2023 12:44
@fallwith fallwith mentioned this pull request Mar 29, 2023
@fallwith
Copy link
Contributor

Hi @wagner, your contribution was included with v9.1.0 of the gem that was just released. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community To tag external issues and PRs submitted by the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants