feat: Skip Trilogyping span with option ping_enabled
#899
Conversation
TonyCTHsu
requested review from
fbogsany,
mwear,
robertlaurin,
dazuma,
ericmustin,
arielvalentin,
ahayworth,
plantfansam,
robbkidd,
simi,
kaylareopelle and
xuan-cao-swi
as code owners
| @@ -28,6 +28,7 @@ class Instrumentation < OpenTelemetry::Instrumentation::Base | |||
| option :span_name, default: :statement_type, validate: %I[statement_type db_name db_operation_and_name] | |||
| option :obfuscation_limit, default: 2000, validate: :integer | |||
| option :propagator, default: nil, validate: :string | |||
| option :ping_enabled, default: true, validate: :boolean | |||
There was a problem hiding this comment.
Looks good to me. I just have two questions based on the original issue
IMO we should default to not tracing ping if it doesn't have a parent and allow a config option to opt in.
- Should this option be false as default?
- I'm not familiar with mysql2 ping operation, so I'm not sure in what case the ping operation will have a parent span. Potentially, a scenario involving a rack span as the parent, with other spans acting as child spans if it's a Rails or Sinatra application invoking ActiveRecord. Do you have any recommendations for addressing cases where a parent span exists and enabling the ping accordingly?
There was a problem hiding this comment.
Addressed with: 8049f2c
- Should this option be false as default?
I set it as true originally because that would result in no data lost and no configuration changes for existing user. I don't mind the option being false. However, I would like to learn whether such change should be considered as a breaking change (major in SemVer2) or behavior change (minor in SemVer2)?
so I'm not sure in what case the ping operation will have a parent span.
I can imagine someone is building a /healthcheck endpoint that implements pinging databases. However, I would feel strange to see the ping span generated if I configured ping_enabled as false (Which also means that where is no way to silence it entirely.)
I am not familiar with OTel's public interfaces but I tried to simulate such scenario in test with
OpenTelemetry.tracer_provider.tracer.in_span('parent') do
client.ping
endLet me know if I am missing something or wrong about this.
There was a problem hiding this comment.
I set it as true originally because that would result in no data lost and no configuration changes for existing user.
Make sense. Maybe ask it in next SIG meeting (also breaking vs behavior change).
I would feel strange to see the ping span generated if I configured ping_enabled as false (Which also means that where is no way to silence it entirely.)
Agree. I think even document it still confuse people. I suggest either removing the condition of if ping doesn't have parent span ... (i.e. disable all if set false) or make the option name more specific.
cc: @plantfansam
| @@ -26,6 +26,8 @@ def initialize(options = {}) | |||
| end | |||
|
|
|||
| def ping(...) | |||
| return super unless OpenTelemetry::Trace.current_span.context.valid? || config[:ping_enabled] | |||
There was a problem hiding this comment.
Is this something that we can mixin during initialization instead of checking every time this method is invoked?
| @@ -28,6 +28,7 @@ class Instrumentation < OpenTelemetry::Instrumentation::Base | |||
| option :span_name, default: :statement_type, validate: %I[statement_type db_name db_operation_and_name] | |||
| option :obfuscation_limit, default: 2000, validate: :integer | |||
| option :propagator, default: nil, validate: :string | |||
| option :ping_enabled, default: false, validate: :boolean | |||
There was a problem hiding this comment.
Like we talked about during the SIG, since this changes the default behavior, keeping default: false will be a breaking change.
If you'd like to keep that approach, please update the PR title to begin with feat!:.
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
|
👋 Hi @TonyCTHsu! It looks like there are two comments in the PR blocking further review: Let us know if you have any follow-up questions! |
|
I think we had a similar discussion where we decided to reject a similar PR for PostgreSQL @robertlaurin suggested using custom samplers instead: |
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
closes #842
Trilogy: Add an configuration option
ping_enabledto skippinginstrumentation