-
Notifications
You must be signed in to change notification settings - Fork 375
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 GraphQL integration reconfiguration #3859
Conversation
@analytics_sample_rate = analytics_sample_rate | ||
|
||
@service_name = service | ||
def initialize(*args, **kwargs) |
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
Optional arguments should appear at the end (...read more)
The rule "Optional arguments should appear at the end" is an important programming practice in Ruby. It is used to ensure that the optional parameters in a method definition are placed after the required parameters. This rule is important because when a method is called, Ruby fills in the arguments from left to right. If an optional argument is placed before a required argument and the method is called with fewer arguments, Ruby will assign the provided arguments to the optional parameters, leaving the required parameters without values and causing an error.
Non-compliance with this rule often leads to unexpected behavior or bugs in the code, which can be quite challenging to debug. This is particularly true when the method is called with fewer arguments than defined. The errors caused by this can be hard to track down, as they may not occur at the place where the method is defined, but rather in some distant place where the method is called.
To avoid this, always place optional parameters at the end of the list of parameters in your method definitions. This way, Ruby will fill in the required parameters first, and only then use any remaining arguments for the optional ones. If there are no remaining arguments, the optional parameters will be set to their default values. This keeps your code clear, predictable, and free of unnecessary bugs.
@analytics_sample_rate = analytics_sample_rate | ||
|
||
@service_name = service | ||
def initialize(*args, **kwargs) |
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
Optional arguments should appear at the end (...read more)
The rule "Optional arguments should appear at the end" is an important programming practice in Ruby. It is used to ensure that the optional parameters in a method definition are placed after the required parameters. This rule is important because when a method is called, Ruby fills in the arguments from left to right. If an optional argument is placed before a required argument and the method is called with fewer arguments, Ruby will assign the provided arguments to the optional parameters, leaving the required parameters without values and causing an error.
Non-compliance with this rule often leads to unexpected behavior or bugs in the code, which can be quite challenging to debug. This is particularly true when the method is called with fewer arguments than defined. The errors caused by this can be hard to track down, as they may not occur at the place where the method is defined, but rather in some distant place where the method is called.
To avoid this, always place optional parameters at the end of the list of parameters in your method definitions. This way, Ruby will fill in the required parameters first, and only then use any remaining arguments for the optional ones. If there are no remaining arguments, the optional parameters will be set to their default values. This keeps your code clear, predictable, and free of unnecessary bugs.
BenchmarksBenchmark execution time: 2024-09-16 23:01:38 Comparing candidate commit 6274db9 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 22 metrics, 2 unstable metrics. scenario:profiler - sample timeline=false
|
def load_test_schema(prefix: '') | ||
# rubocop:disable Security/Eval | ||
# rubocop:disable Style/DocumentDynamicEvalDefinition | ||
eval <<-RUBY, binding, __FILE__, __LINE__ + 1 |
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
Avoid using SHA1, it is known to be vulnerable (...read more)
The eval
method in Ruby is used to execute a string of code at runtime, essentially treating it as a part of the program. While powerful, it exposes your code to significant security risks, as it can execute any code it's given. This includes potentially harmful code that can alter or delete data, or interact with the system on which your Ruby program is running.
The use of eval
is considered a bad practice because it can lead to code injection attacks. An attacker can inject malicious code into the string that eval
will execute. This can lead to a variety of security vulnerabilities, such as unauthorized access to sensitive data, corruption of data, or even taking control of the entire system.
Instead of using eval
, consider using safer alternatives like send
or public_send
. These methods allow you to call methods dynamically on objects without the security risks associated with eval
. If you need to execute dynamically generated code, consider using the RubyVM::InstructionSequence
class, which can compile and execute code in a safer manner. Always validate and sanitize any user input that will be used in these methods to prevent code injection attacks.
6dac053
to
9083dae
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3859 +/- ##
==========================================
- Coverage 97.85% 97.79% -0.07%
==========================================
Files 1285 1282 -3
Lines 76951 76871 -80
Branches 3789 3791 +2
==========================================
- Hits 75303 75176 -127
- Misses 1648 1695 +47 ☔ View full report in Codecov by Sentry. |
a5b0370
to
6274db9
Compare
This PR fixes an issue where it is not possible to reconfigure the GraphQL integration.
This is especially important when using auto instrumentation (
gem 'datadog', require: 'datadog/auto_instrument'
) because any configuration inside aDatadog.configure{}
block in an auto instrumented application is in fact a reconfiguration.The main use case for this PR is supporting changing the service name for GraphQL spans.
Additional Notes:
One important caveat I found while working on this PR is that it is not possible to change GraphQL instrumentation strategies at runtime, as instrumentation pollutes internal GraphQL classes in irreversible ways.
This was already the case before this PR, and I was not able to address this without a breaking change, thus this limitation remains.
I add public documentation about this in the GettingStarted guide.
I also slip the GraphQL test in the Rakefile to ensure conflicting instrumentation strategies do not run in the same process. This was not required before due to precise class manipulation surgery, but it became way too complex when I tried to adapt the existing class manipulation to this PR.
I added my recommendation on how to address this limitation, during the next major release, in inline comments.
How to test the change?
There are integration tests for all the changes.
Unsure? Have a question? Request a review!