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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 2 commits into from
+250 −453

Conversation

Strech
Copy link
Contributor

@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 •
edited
Loading

Benchmarks

Benchmark execution time: 2024-11-21 14:00:16

Comparing candidate commit dfe942e in PR branch appsec-55936-remove-reactive-operation with baseline commit 0dec342 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 •
edited
Loading

Codecov Report

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

Project coverage is 97.78%. Comparing base (b0b8015) to head (dfe942e).
Report is 4 commits behind head on master.

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.78%   97.78%           
=======================================
  Files        1350     1348    -2     
  Lines       81322    81232   -90     
  Branches     4107     4104    -3     
=======================================
- Hits        79522    79434   -88     
+ Misses       1800     1798    -2

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

🚨 Try these New Features:

@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
vpellan
vpellan approved these changes Nov 21, 2024
View reviewed changes
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

def self.publish: (Datadog::AppSec::Reactive::Operation op, Datadog::AppSec::Contrib::GraphQL::Gateway::Multiplex gateway_multiplex) -> boolish
def self.publish: (AppSec::Reactive::Engine op, AppSec::Contrib::GraphQL::Gateway::Multiplex gateway_multiplex) -> boolish
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we keep the variable name for AppSec::Reactive::Engine to op (including in the lib folder for the corresponding methods) or rename it to engine ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vpellan vpellan vpellan approved these changes

@y9v y9v y9v approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Strech @codecov-commenter @y9v @vpellan