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

[APPSEC-55936] Remove Reactive::Operation #4138

Merged
merged 4 commits into from
Dec 12, 2024

Conversation

Strech
Copy link
Member

@Strech Strech commented Nov 21, 2024

What does this PR do?

Remove Reactive::Operation class as irresponsible proxy.

Motivation:

After analysis we decide to move forward without existing reactivity and gateway. Decision made based on the fact that implementation is not finished and subpar on complexity and potentially performance compared to benefits it gives us right now.

Change log entry

No.

Additional Notes:

This is a first step in the direction of simplification of the AppSec instrumentation. To gain some speed we need to trim the deadweight.

How to test the change?

CI is enough.

@Strech Strech added appsec Application Security monitoring product dev/internal Other internal work that does not need to be included in the changelog labels Nov 21, 2024
@github-actions github-actions bot added the integrations Involves tracing integrations label Nov 21, 2024
@Strech Strech added dev/refactor Involves refactoring existing components and removed integrations Involves tracing integrations dev/refactor Involves refactoring existing components labels Nov 21, 2024
@Strech Strech force-pushed the appsec-55936-remove-reactive-operation branch from 2ff17b8 to 332b8fc Compare November 21, 2024 11:03
@github-actions github-actions bot added the integrations Involves tracing integrations label Nov 21, 2024
@Strech Strech force-pushed the appsec-55936-remove-reactive-operation branch from 332b8fc to d79b926 Compare November 21, 2024 11:10
@pr-commenter
Copy link

pr-commenter bot commented Nov 21, 2024

Benchmarks

Benchmark execution time: 2024-12-11 11:59:45

Comparing candidate commit 220c0a1 in PR branch appsec-55936-remove-reactive-operation with baseline commit b985e32 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 31 metrics, 2 unstable metrics.

@Strech Strech force-pushed the appsec-55936-remove-reactive-operation branch from d79b926 to 5b9ee8b Compare November 21, 2024 12:35
@Strech Strech removed the integrations Involves tracing integrations label Nov 21, 2024
@Strech Strech force-pushed the appsec-55936-remove-reactive-operation branch from 5b9ee8b to 667938d Compare November 21, 2024 12:44
@github-actions github-actions bot added the integrations Involves tracing integrations label Nov 21, 2024
@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2024

Codecov Report

Attention: Patch coverage is 96.98276% with 7 lines in your changes missing coverage. Please review.

Project coverage is 97.76%. Comparing base (b985e32) to head (220c0a1).

Files with missing lines Patch % Lines
lib/datadog/appsec/contrib/rack/gateway/watcher.rb 91.89% 3 Missing ⚠️
.../datadog/appsec/contrib/sinatra/gateway/watcher.rb 92.00% 2 Missing ⚠️
...ib/datadog/appsec/contrib/rails/gateway/watcher.rb 90.00% 1 Missing ⚠️
lib/datadog/appsec/monitor/gateway/watcher.rb 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4138      +/-   ##
==========================================
+ Coverage   97.75%   97.76%   +0.01%     
==========================================
  Files        1357     1355       -2     
  Lines       82130    82040      -90     
  Branches     4174     4171       -3     
==========================================
- Hits        80288    80210      -78     
+ Misses       1842     1830      -12     

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

@Strech Strech marked this pull request as ready for review November 21, 2024 13:07
@Strech Strech requested review from a team as code owners November 21, 2024 13:07
@Strech Strech force-pushed the appsec-55936-remove-reactive-operation branch from 667938d to dfe942e Compare November 21, 2024 13:16
Copy link
Contributor

@vpellan vpellan left a comment

Choose a reason for hiding this comment

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

I just have one non-blocking suggestion, other than that LGTM!

sig/datadog/appsec/contrib/graphql/reactive/multiplex.rbs Outdated Show resolved Hide resolved
@Strech Strech force-pushed the appsec-55936-remove-reactive-operation branch from dfe942e to c2f8ac8 Compare November 22, 2024 10:17
@Strech Strech force-pushed the appsec-55936-remove-reactive-operation branch from c2f8ac8 to 5e3a4b0 Compare November 22, 2024 10:23
@ivoanjo
Copy link
Member

ivoanjo commented Dec 9, 2024

Hey @Strech this one looks still relevant to merge in at some point? :)

@Strech
Copy link
Member Author

Strech commented Dec 11, 2024

@ivoanjo Yes, we want to shrink the code, but I'm missing approval of @Datadog/apm-idm-ruby

P.S @marcotc may I ask you for the review?

@datadog-datadog-prod-us1
Copy link
Contributor

datadog-datadog-prod-us1 bot commented Dec 11, 2024

Datadog Report

Branch report: appsec-55936-remove-reactive-operation
Commit report: 220c0a1
Test service: dd-trace-rb

✅ 0 Failed, 22063 Passed, 1457 Skipped, 5m 24.89s Total Time

@Strech Strech merged commit 3c0de7d into master Dec 12, 2024
338 of 339 checks passed
@Strech Strech deleted the appsec-55936-remove-reactive-operation branch December 12, 2024 08:42
@github-actions github-actions bot added this to the 2.9.0 milestone Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
appsec Application Security monitoring product dev/internal Other internal work that does not need to be included in the changelog integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants