-
Notifications
You must be signed in to change notification settings - Fork 383
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
Added: Rails 8.0 support #4455
base: master
Are you sure you want to change the base?
Added: Rails 8.0 support #4455
Conversation
Thank you for updating Change log entry section 👏 Visited at: 2025-03-25 18:39:46 UTC |
@@ -139,7 +139,8 @@ def index | |||
end | |||
end | |||
|
|||
if Rails.version >= '4' | |||
# Lograge does not support tagged logging, the default logger since Rails 5 | |||
if Rails.version >= '4' && Rails.version < '5' |
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.
⚪ Code Quality Violation
Consider using ranges or between to simplify your comparison (...read more)
The rule "Prefer ranges/between over complex comparisons" advises developers to use the range or between?
method for comparisons instead of complex conditional statements. This practice increases the readability and clarity of your code. Complex comparisons using logical operators can be difficult to understand and prone to errors.
This rule is important because it promotes cleaner, more efficient, and easier-to-read code. When code is easier to read, it's easier to maintain, debug, and less likely to contain hidden bugs. Using the range or between?
method is a more concise way to check if a value falls within a specific range.
To adhere to this rule, replace complex comparison statements with the range or between?
method. For example, instead of writing foo >= 42 && foo <= 99
, you can write (42..99).include?(foo)
or foo.between?(42, 99)
. These alternatives are more straightforward and visually cleaner, making your code easier to understand.
@@ -96,6 +99,9 @@ def set_cache_key(span, key, multi_key) | |||
cache_key = Core::Utils.truncate(resolved_key, Ext::QUANTIZE_CACHE_MAX_KEY_SIZE) | |||
span.set_tag(Ext::TAG_CACHE_KEY_MULTI, cache_key) | |||
else | |||
# `dd_original_keys` is an Array with one element when `multi_key` is false. | |||
# But if using the fallback `key`, it is a simple scalar value. | |||
key = key.is_a?(Array) ? key[0] : key |
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.
⚪ Code Quality Violation
Consider using Array() to ensure the type is that of an array (...read more)
The rule "Use Array()
to ensure your variable is an array" is important for ensuring your code behaves as expected, regardless of the type of data it receives. It is common in Ruby to need to iterate through an array of items. However, if the variable is not an array, this can lead to unexpected behavior or errors.
The Array()
method in Ruby is a Kernel method that converts its argument to an Array. If the argument is already an Array, it returns the argument. If the argument is nil, it returns an empty Array. This can be used to ensure that a variable is an array before trying to iterate over it, preventing potential errors or unexpected behavior.
By using Array(foos)
, you can ensure that foos
is an array before you try to iterate over it with each
. This prevents the need to check if foos
is an array with foos.is_a?(Array)
and makes your code cleaner and easier to understand.
@@ -488,7 +489,8 @@ def index | |||
end | |||
end | |||
|
|||
if Rails.version >= '4' | |||
# Lograge does not support tagged logging, the default logger since Rails 5 | |||
if Rails.version >= '4' && Rails.version < '5' |
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.
⚪ Code Quality Violation
Consider using ranges or between to simplify your comparison (...read more)
The rule "Prefer ranges/between over complex comparisons" advises developers to use the range or between?
method for comparisons instead of complex conditional statements. This practice increases the readability and clarity of your code. Complex comparisons using logical operators can be difficult to understand and prone to errors.
This rule is important because it promotes cleaner, more efficient, and easier-to-read code. When code is easier to read, it's easier to maintain, debug, and less likely to contain hidden bugs. Using the range or between?
method is a more concise way to check if a value falls within a specific range.
To adhere to this rule, replace complex comparison statements with the range or between?
method. For example, instead of writing foo >= 42 && foo <= 99
, you can write (42..99).include?(foo)
or foo.between?(42, 99)
. These alternatives are more straightforward and visually cleaner, making your code easier to understand.
spec/support/execute_in_fork.rb
Outdated
_, status = Process.wait2(pid) | ||
|
||
while (read_size = reader.gets) | ||
call = Marshal.load(reader.read(Integer(read_size))) |
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.
🟠 Code Vulnerability
Potential unsafe deserialization. Ensure there is no potential data injection. (...read more)
This rule advises against the use of unsafe deserialization in Ruby, particularly with the Marshal.load
method. Deserialization is the process of converting data from a binary or string format back into an object. However, if the data was tampered with, it could lead to arbitrary code execution when the data is deserialized.
This is important because it can lead to serious security vulnerabilities. An attacker could exploit the deserialization process to execute malicious code, alter program flow, or perform other harmful actions. This is particularly dangerous if your application runs with high privileges.
To avoid this, use safe deserialization methods. Instead of using Marshal.load
, consider using JSON or YAML for serialization and deserialization, as they are safer. For example, you could use JSON.parse(data)
or YAML.load(data)
instead. Additionally, always ensure that the data you are deserializing comes from a trusted source.
spec/support/execute_in_fork.rb
Outdated
# puts("call: #{call}") | ||
|
||
if call[:receiver] == :reporter | ||
reporter.send(call[:method], *args) |
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.
⚪ Code Quality Violation
reporter.send(call[:method], *args) | |
reporter.send(call.fetch(:method), *args) |
Consider using fetch on hash key (...read more)
The rule "Use fetch to check hash keys" encourages the use of the fetch
method over the direct hash access method []
for checking hash keys. This is because fetch
raises an error when the key does not exist in the hash, making the code more robust and fail-safe by preventing any unexpected behavior due to missing hash keys.
The significance of this rule lies in its ability to make the code more predictable and error-resistant. When a hash key is accessed directly using []
, and the key does not exist, Ruby returns nil
by default. This can lead to subtle bugs if the existence of the key is crucial for the subsequent code. Using fetch
, on the other hand, will raise a KeyError
if the key is not found, making it immediately clear that there's an issue with the code.
Adhering to this rule is straightforward. Instead of using direct hash access, use the fetch
method whenever you need to access a hash key. For example, instead of hash[:key]
, use hash.fetch(:key)
. This way, if the key does not exist in the hash, your code will raise an error, allowing you to catch and handle the problem early on.
spec/support/execute_in_fork.rb
Outdated
if call[:receiver] == :reporter | ||
reporter.send(call[:method], *args) | ||
elsif call[:receiver] == :execution_result | ||
execution_result.send(call[:method], *args) |
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.
⚪ Code Quality Violation
execution_result.send(call[:method], *args) | |
execution_result.send(call.fetch(:method), *args) |
Consider using fetch on hash key (...read more)
The rule "Use fetch to check hash keys" encourages the use of the fetch
method over the direct hash access method []
for checking hash keys. This is because fetch
raises an error when the key does not exist in the hash, making the code more robust and fail-safe by preventing any unexpected behavior due to missing hash keys.
The significance of this rule lies in its ability to make the code more predictable and error-resistant. When a hash key is accessed directly using []
, and the key does not exist, Ruby returns nil
by default. This can lead to subtle bugs if the existence of the key is crucial for the subsequent code. Using fetch
, on the other hand, will raise a KeyError
if the key is not found, making it immediately clear that there's an issue with the code.
Adhering to this rule is straightforward. Instead of using direct hash access, use the fetch
method whenever you need to access a hash key. For example, instead of hash[:key]
, use hash.fetch(:key)
. This way, if the key does not exist in the hash, your code will raise an error, allowing you to catch and handle the problem early on.
spec/support/execute_in_fork.rb
Outdated
raise "Unknown receiver: #{call[:receiver]}" | ||
end | ||
|
||
receiver.send(call[:method], *args) |
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.
⚪ Code Quality Violation
receiver.send(call[:method], *args) | |
receiver.send(call.fetch(:method), *args) |
Consider using fetch on hash key (...read more)
The rule "Use fetch to check hash keys" encourages the use of the fetch
method over the direct hash access method []
for checking hash keys. This is because fetch
raises an error when the key does not exist in the hash, making the code more robust and fail-safe by preventing any unexpected behavior due to missing hash keys.
The significance of this rule lies in its ability to make the code more predictable and error-resistant. When a hash key is accessed directly using []
, and the key does not exist, Ruby returns nil
by default. This can lead to subtle bugs if the existence of the key is crucial for the subsequent code. Using fetch
, on the other hand, will raise a KeyError
if the key is not found, making it immediately clear that there's an issue with the code.
Adhering to this rule is straightforward. Instead of using direct hash access, use the fetch
method whenever you need to access a hash key. For example, instead of hash[:key]
, use hash.fetch(:key)
. This way, if the key does not exist in the hash, your code will raise an error, allowing you to catch and handle the problem early on.
# These come in as method calls, which we replicate here in the parent process. | ||
while (read_size = reader.gets) # Reads a header line, containing the size of the next object | ||
# Read the next object | ||
call = Marshal.load(reader.read(Integer(read_size))) |
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.
🟠 Code Vulnerability
Potential unsafe deserialization. Ensure there is no potential data injection. (...read more)
This rule advises against the use of unsafe deserialization in Ruby, particularly with the Marshal.load
method. Deserialization is the process of converting data from a binary or string format back into an object. However, if the data was tampered with, it could lead to arbitrary code execution when the data is deserialized.
This is important because it can lead to serious security vulnerabilities. An attacker could exploit the deserialization process to execute malicious code, alter program flow, or perform other harmful actions. This is particularly dangerous if your application runs with high privileges.
To avoid this, use safe deserialization methods. Instead of using Marshal.load
, consider using JSON or YAML for serialization and deserialization, as they are safer. For example, you could use JSON.parse(data)
or YAML.load(data)
instead. Additionally, always ensure that the data you are deserializing comes from a trusted source.
defined?(::ActiveSupport::Cache::RedisCacheStore) && | ||
# ... to check that we need to call `autoload?` and check if it returns `nil`, meaning it's loaded. | ||
::ActiveSupport::Cache.autoload?(:RedisCacheStore).nil? && | ||
Support.autoloaded?(::ActiveSupport::Cache, :RedisCacheStore) && |
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.
I refactored this for a different part of this PR, but I didn't actually need it there, but I left it here because it's an autoload
gotcha.
@@ -132,7 +132,9 @@ def setup(config) | |||
# Semantic Logger settings should be exclusive to `ActiveSupport::TaggedLogging` and `Lograge` | |||
if config.respond_to?(:rails_semantic_logger) | |||
config.rails_semantic_logger.add_file_appender = false | |||
config.semantic_logger.add_appender(logger: config.logger) | |||
ThreadHelpers.with_leaky_thread_creation('semantic_logger log') do |
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 like we missed labelling this one before.
@@ -149,7 +150,7 @@ | |||
# Thread has shut down, but we caught it right as it was still alive | |||
!t.alive? || | |||
# Long-lived Timeout thread created by `Timeout.create_timeout_thread`. | |||
(t.respond_to?(:name) && t.name == 'Timeout stdlib thread') || | |||
t.name == 'Timeout stdlib thread' || |
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.
Clean up, as Ruby 2.5+ always have Thread#name
.
'RackStatusApp', | ||
Class.new do | ||
def call(_env) | ||
[200, { 'Content-Type' => 'text/plain' }, ['OK']] |
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.
⚪ Code Quality Violation
[200, { 'Content-Type' => 'text/plain' }, ['OK']] | |
[200, { 'Content-Type' => 'text/plain' }, %w[OK]] |
Consider using the %w syntax instead (...read more)
The rule "Prefer %w
to the literal array syntax" is a Ruby style guideline that encourages the use of %w
notation instead of the traditional array syntax when defining arrays of strings. This rule is part of the Ruby community's efforts to promote readability and simplicity in Ruby code.
This rule is important because it helps to keep the code concise and easy to read. The %w
notation allows you to define an array of strings without having to use quotes and commas. This can make the code cleaner and easier to understand, especially when dealing with large arrays.
To follow this rule, replace the traditional array syntax with the %w
notation. For example, instead of writing ['foo', 'bar', 'baz']
, you should write %w[foo bar baz]
. This will create the same array, but in a more readable and concise way. By following this rule, you can help to make your Ruby code cleaner and easier to understand.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4455 +/- ##
==========================================
- Coverage 97.76% 97.65% -0.12%
==========================================
Files 1391 1395 +4
Lines 84822 85033 +211
Branches 4281 4327 +46
==========================================
+ Hits 82929 83036 +107
- Misses 1893 1997 +104 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
What does this PR do?
Adds support for all existing instrumentation for Rails 8.0.x.
The only production change is in how ActiveSupport cache keys are reported in a span (there's some special handling added for Rails 8), but other parts stay identical.
The tricky part was getting CI to run: Rails 8 really doesn't like to be restarted in-memory, like we do for all other versions of Rails. I had to resort to forking on each test run, something that is not actually supported by RSpec, so I added support for it in
spec/support/execute_in_fork.rb
.Change log entry
Yes. Add support for Rails 8.0.
Additional Notes:
It was soooo hard to get the tests to run, without making a complete and utter mess in the process.
How to test the change?
All changes have automated tests.