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

Remove opentracing support #3443

Merged
merged 3 commits into from
Feb 14, 2024
Merged

Remove opentracing support #3443

merged 3 commits into from
Feb 14, 2024

Conversation

TonyCTHsu
Copy link
Contributor

@TonyCTHsu TonyCTHsu commented Feb 7, 2024

2.0 Upgrade Guide notes

OpenTracing is no longer supported from 2+.

@TonyCTHsu TonyCTHsu changed the base branch from master to 2.0 February 7, 2024 10:31
@TonyCTHsu TonyCTHsu force-pushed the tonycthsu/remove-opentracing branch from ec2ac94 to d3eeb98 Compare February 8, 2024 08:58
@github-actions github-actions bot added appsec Application Security monitoring product integrations Involves tracing integrations tracing labels Feb 8, 2024
@TonyCTHsu TonyCTHsu force-pushed the tonycthsu/remove-opentracing branch from d3eeb98 to c0d0341 Compare February 8, 2024 10:06
@TonyCTHsu TonyCTHsu marked this pull request as ready for review February 8, 2024 10:10
@TonyCTHsu TonyCTHsu requested review from a team as code owners February 8, 2024 10:10
@TonyCTHsu
Copy link
Contributor Author

options['tracing.opentracing.enabled'] = !defined?(Datadog::OpenTracer::LOADED).nil?

Thoughts on this?

  1. Remove the entire key value pair
  2. Fixate with the value false
  3. Leave it as it is, value will be evaluated as false

@@ -51,7 +51,6 @@ def before_initialize(app)
BEFORE_INITIALIZE_ONLY_ONCE_PER_APP[app].run do
# Middleware must be added before the application is initialized.
# Otherwise the middleware stack will be frozen.
# Sometimes we don't want to activate middleware e.g. OpenTracing, etc.
add_middleware(app) if Datadog.configuration.tracing[:rails][:middleware]
Copy link
Member

Choose a reason for hiding this comment

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

Does Datadog.configuration.tracing[:rails][:middleware] == false ever make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking where you want the other Rails instrumentation but not the Rack middleware. Very niche, but possible. I'd look at the history of the option before making too many assumptions, though.

# Sometimes we don't want to activate middleware e.g. OpenTracing, etc.
add_middleware(app) if Datadog.configuration.tracing[:rails][:middleware]
Copy link
Member

Choose a reason for hiding this comment

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

Does Datadog.configuration.tracing[:rails][:middleware] == false ever make sense?

@marcotc
Copy link
Member

marcotc commented Feb 8, 2024

options['tracing.opentracing.enabled'] = !defined?(Datadog::OpenTracer::LOADED).nil?

Thoughts on this?

1. Remove the entire key value pair

2. Fixate with the value `false`

3. Leave it as it is, value will be evaluated as `false`

Remove it, it's not even parsed today by our backend.

@github-actions github-actions bot added the core Involves Datadog core libraries label Feb 13, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cfe5486) 98.10% compared to head (787d898) 98.07%.

Additional details and impacted files
@@            Coverage Diff             @@
##              2.0    #3443      +/-   ##
==========================================
- Coverage   98.10%   98.07%   -0.03%     
==========================================
  Files        1262     1229      -33     
  Lines       73316    71801    -1515     
  Branches     3434     3379      -55     
==========================================
- Hits        71923    70416    -1507     
+ Misses       1393     1385       -8     

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

@TonyCTHsu TonyCTHsu merged commit a6ca91d into 2.0 Feb 14, 2024
153 checks passed
@TonyCTHsu TonyCTHsu deleted the tonycthsu/remove-opentracing branch February 14, 2024 14:59
@TonyCTHsu TonyCTHsu added the 2.0 label Feb 20, 2024
@TonyCTHsu TonyCTHsu added this to the 2.0 milestone Feb 20, 2024
@TonyCTHsu TonyCTHsu modified the milestones: 2.0, 2.0.0.beta1 Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0 appsec Application Security monitoring product core Involves Datadog core libraries integrations Involves tracing integrations tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants