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

Fix ActiveRecord resolve function #3523

Merged
merged 1 commit into from
Mar 18, 2024

Conversation

TonyCTHsu
Copy link
Contributor

@TonyCTHsu TonyCTHsu commented Mar 14, 2024

closes #3522

What does this PR do?

When an invalid string is provided for multiplexing configuration, although it logs an error, it still add the matcher true to the configuration. Later during resolving, an exception will be raised.

In this PR, the mitigation is to skip adding the matcher if the input is invalid, also reduce the log message.

@github-actions github-actions bot added integrations Involves tracing integrations tracing labels Mar 14, 2024
@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2024

Codecov Report

Attention: Patch coverage is 96.42857% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 98.24%. Comparing base (6b85e19) to head (c3fe261).

Files Patch % Lines
...dog/tracing/contrib/active_record/multi_db_spec.rb 88.88% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3523   +/-   ##
=======================================
  Coverage   98.23%   98.24%           
=======================================
  Files        1275     1275           
  Lines       75303    75329   +26     
  Branches     3558     3559    +1     
=======================================
+ Hits        73977    74004   +27     
+ Misses       1326     1325    -1     

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

@TonyCTHsu TonyCTHsu force-pushed the tonycthsu/fix-activerecord-resolving branch from 4b47bd6 to c3fe261 Compare March 14, 2024 22:12
)

nil
Copy link
Contributor Author

@TonyCTHsu TonyCTHsu Mar 14, 2024

Choose a reason for hiding this comment

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

It is important to return nil, otherwise, true is returned, causing exception during resolving

@TonyCTHsu TonyCTHsu self-assigned this Mar 14, 2024
@TonyCTHsu TonyCTHsu marked this pull request as ready for review March 14, 2024 22:30
@TonyCTHsu TonyCTHsu requested a review from a team as a code owner March 14, 2024 22:30
@TonyCTHsu TonyCTHsu added this to the 1.21.1 milestone Mar 15, 2024
@TonyCTHsu TonyCTHsu merged commit f247bae into master Mar 18, 2024
219 checks passed
@TonyCTHsu TonyCTHsu deleted the tonycthsu/fix-activerecord-resolving branch March 18, 2024 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrations Involves tracing integrations tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ActiveRecord describes: with an invalid value logs full database config
3 participants