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

Extract db appraisal group #3130

Merged
merged 21 commits into from
Sep 15, 2023
Merged

Conversation

TonyCTHsu
Copy link
Contributor

@TonyCTHsu TonyCTHsu commented Sep 14, 2023

Motivation:

From internal discussion, we want to horizontally scale our appraisal groups, instead of vertically. This means we are breaking large appraisal groups into smaller ones.

What does this PR do?

This PR extracts relational_db group from contrib group. This group includes a range of gems that are depending on a relational database.

Additional Notes

I started off with extracting activesupport group, but found there are strong coupling in our test with our db adatpers(mysql2, pg and sqlite) while activerecord does not declare dependencies for them. It would be strange to include those adapters into the activesupport group. Hence, I pivot to extract those relational database related gems. I hesitate to include other db related gems(mongodb, presto/trino, dalli) at this moment.

The original bundle exec rake test:autoinstrument is failing because the test cases were setup with a Sinatra application making db request but now the appraisal group contrib no longer contains those db gems. I consider making db request to be excessive for test case to verify the auto instrument behaviour, so I fix the task by removing the those db calls and change the dependent group to sinatra and increase the ruby version support to latest version.

@TonyCTHsu TonyCTHsu marked this pull request as ready for review September 14, 2023 07:29
@TonyCTHsu TonyCTHsu requested a review from a team September 14, 2023 07:29
@GustavoCaso
Copy link
Member

I hesitate to include other db related gems(mongodb, presto/trino, dalli) at this moment.

Can you explain why you hesitated?

@TonyCTHsu
Copy link
Contributor Author

I hesitate to include other db related gems(mongodb, presto/trino, dalli) at this moment.

Can you explain why you hesitated?

The reason I group those gems because they require database adapter (mysql2, pg and sqlite) to work properly. With such group, I can test the instrumentation for high level ORM (active_record, sequel and delayed_job ) and instrumentation for low level drivers (mysql2, pg and sqlite).

The ones excluded (mongodb, presto/trino, dalli) do not required those drivers or they contain their own drivers.

@TonyCTHsu TonyCTHsu merged commit 8963e01 into master Sep 15, 2023
176 checks passed
@TonyCTHsu TonyCTHsu deleted the tonycthsu/extract-db-appraisal-group branch September 15, 2023 09:31
@github-actions github-actions bot added this to the 1.15.0 milestone Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants