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

feat(Integrations): update user login and signup events collection #2976

Merged
merged 22 commits into from
Jan 20, 2025

Conversation

Leiyks
Copy link
Contributor

@Leiyks Leiyks commented Nov 27, 2024

Description

Implement changes on user login and signup automated event collection according to the RFC.

The following changes have been implemented:

  • Separate event collection methods to have an automated version and a SDK version.
  • Collect login information and send them via the appropriate addresses on the automated event collection methods.
  • Add missing fingerprint addresses appsec.events.users.login.failure and appsec.events.users.login.success
  • Update the Wordpress, Laravel and Symphony integration to use new automated methods.

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

Related Jiras: APPSEC-55606, APPSEC-55605, APPSEC-55604

@codecov-commenter
Copy link

codecov-commenter commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 78.13765% with 54 lines in your changes missing coverage. Please review.

Project coverage is 73.02%. Comparing base (6b3218e) to head (00c42f0).

Files with missing lines Patch % Lines
appsec/src/extension/tags.c 80.79% 19 Missing and 15 partials ⚠️
...DTrace/Integrations/Laravel/LaravelIntegration.php 56.00% 11 Missing ⚠️
...DTrace/Integrations/Symfony/SymfonyIntegration.php 66.66% 9 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2976      +/-   ##
============================================
- Coverage     74.80%   73.02%   -1.78%     
- Complexity     2784     2787       +3     
============================================
  Files           112      139      +27     
  Lines         11020    15272    +4252     
  Branches          0     1043    +1043     
============================================
+ Hits           8243    11152    +2909     
- Misses         2777     3569     +792     
- Partials          0      551     +551     
Flag Coverage Δ
appsec-extension 68.41% <81.00%> (?)
tracer-php 74.79% <70.58%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
appsec/src/extension/user_tracking.c 70.75% <100.00%> (ø)
...ce/Integrations/WordPress/WordPressIntegration.php 94.11% <100.00%> (+0.29%) ⬆️
...DTrace/Integrations/Symfony/SymfonyIntegration.php 82.71% <66.66%> (-0.14%) ⬇️
...DTrace/Integrations/Laravel/LaravelIntegration.php 80.22% <56.00%> (-0.53%) ⬇️
appsec/src/extension/tags.c 80.85% <80.79%> (ø)

... and 25 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b3218e...00c42f0. Read the comment docs.

@pr-commenter
Copy link

pr-commenter bot commented Nov 27, 2024

Benchmarks [ tracer ]

Benchmark execution time: 2025-01-06 16:46:28

Comparing candidate commit 5930a5e in PR branch leiyks/add-laravel-login-and-signup-collection with baseline commit 205c6d8 in branch master.

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

scenario:HookBench/benchWithoutHook-opcache

  • 🟩 execution_time [-3.728µs; -1.885µs] or [-6.982%; -3.530%]

scenario:LogsInjectionBench/benchLogsInfoBaseline-opcache

  • 🟥 execution_time [+255.790ns; +289.810ns] or [+16.413%; +18.595%]

scenario:LogsInjectionBench/benchLogsInfoInjection-opcache

  • 🟥 execution_time [+255.724ns; +626.676ns] or [+2.705%; +6.628%]

scenario:LogsInjectionBench/benchLogsNullBaseline-opcache

  • 🟩 execution_time [-737.252ns; -598.748ns] or [-3.431%; -2.786%]

scenario:TraceSerializationBench/benchSerializeTrace

  • 🟩 execution_time [-12.079µs; -6.221µs] or [-5.811%; -2.993%]

@Leiyks Leiyks force-pushed the leiyks/add-laravel-login-and-signup-collection branch from 95dcd66 to 22d8216 Compare December 3, 2024 15:00
@pr-commenter
Copy link

pr-commenter bot commented Dec 3, 2024

Benchmarks [ appsec ]

Benchmark execution time: 2024-12-24 14:21:13

Comparing candidate commit 3df244c in PR branch leiyks/add-laravel-login-and-signup-collection with baseline commit 0d57b55 in branch master.

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

@Leiyks Leiyks force-pushed the leiyks/add-laravel-login-and-signup-collection branch 4 times, most recently from dfb19df to 50499a4 Compare December 10, 2024 13:00
@Leiyks Leiyks changed the title feat(Integrations: Laravel): update user login and signup events feat(Integrations): update user lifecycle tracking events Dec 11, 2024
@Leiyks Leiyks force-pushed the leiyks/add-laravel-login-and-signup-collection branch from 2d3ddb4 to ef44aac Compare December 11, 2024 17:11
@Leiyks Leiyks changed the title feat(Integrations): update user lifecycle tracking events feat(Integrations): update user login and signup events collection Dec 11, 2024
@Leiyks Leiyks marked this pull request as ready for review December 12, 2024 10:02
@Leiyks Leiyks requested review from a team as code owners December 12, 2024 10:02
@Leiyks Leiyks force-pushed the leiyks/add-laravel-login-and-signup-collection branch from ee0b645 to 99c0519 Compare December 19, 2024 14:55
appsec/src/extension/tags.c Outdated Show resolved Hide resolved
appsec/src/extension/tags.c Outdated Show resolved Hide resolved
appsec/src/extension/tags.c Show resolved Hide resolved
appsec/src/extension/tags.c Outdated Show resolved Hide resolved
appsec/src/extension/tags.c Outdated Show resolved Hide resolved
appsec/src/extension/tags.c Outdated Show resolved Hide resolved
appsec/src/extension/tags.c Show resolved Hide resolved
appsec/tests/extension/headers_collection_14.phpt Outdated Show resolved Hide resolved
src/DDTrace/Integrations/Laravel/LaravelIntegration.php Outdated Show resolved Hide resolved
tests/Appsec/Mock.php Show resolved Hide resolved
@Leiyks Leiyks force-pushed the leiyks/add-laravel-login-and-signup-collection branch 3 times, most recently from 8296e70 to 3df244c Compare December 24, 2024 13:44
@Leiyks Leiyks force-pushed the leiyks/add-laravel-login-and-signup-collection branch 8 times, most recently from 72f6119 to 8c2852e Compare January 7, 2025 16:25
@Leiyks Leiyks force-pushed the leiyks/add-laravel-login-and-signup-collection branch from bdf213e to 7f63405 Compare January 17, 2025 09:54
Copy link
Contributor

@PROFeNoM PROFeNoM left a comment

Choose a reason for hiding this comment

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

I believe that the trailing commas are the reason for the failing tests in PHP 7.2 and earlier

src/DDTrace/Integrations/Symfony/SymfonyIntegration.php Outdated Show resolved Hide resolved
src/DDTrace/Integrations/Symfony/SymfonyIntegration.php Outdated Show resolved Hide resolved
src/DDTrace/Integrations/Symfony/SymfonyIntegration.php Outdated Show resolved Hide resolved
src/DDTrace/Integrations/Laravel/LaravelIntegration.php Outdated Show resolved Hide resolved
src/DDTrace/Integrations/Laravel/LaravelIntegration.php Outdated Show resolved Hide resolved
Leiyks added 21 commits January 20, 2025 10:46
Signed-off-by: Alexandre Rulleau <alexandre.rulleau@datadoghq.com>
Signed-off-by: Alexandre Rulleau <alexandre.rulleau@datadoghq.com>
…p event

Signed-off-by: Alexandre Rulleau <alexandre.rulleau@datadoghq.com>
… failure event

Signed-off-by: Alexandre Rulleau <alexandre.rulleau@datadoghq.com>
… success event

Signed-off-by: Alexandre Rulleau <alexandre.rulleau@datadoghq.com>
…n user events

Signed-off-by: Alexandre Rulleau <alexandre.rulleau@datadoghq.com>
Signed-off-by: Alexandre Rulleau <alexandre.rulleau@datadoghq.com>
…nctions

Signed-off-by: Alexandre Rulleau <alexandre.rulleau@datadoghq.com>
Signed-off-by: Alexandre Rulleau <alexandre.rulleau@datadoghq.com>
Signed-off-by: Alexandre Rulleau <alexandre.rulleau@datadoghq.com>
Signed-off-by: Alexandre Rulleau <alexandre.rulleau@datadoghq.com>
Signed-off-by: Alexandre Rulleau <alexandre.rulleau@datadoghq.com>
Signed-off-by: Alexandre Rulleau <alexandre.rulleau@datadoghq.com>
Signed-off-by: Alexandre Rulleau <alexandre.rulleau@datadoghq.com>
Signed-off-by: Alexandre Rulleau <alexandre.rulleau@datadoghq.com>
Signed-off-by: Alexandre Rulleau <alexandre.rulleau@datadoghq.com>
Signed-off-by: Alexandre Rulleau <alexandre.rulleau@datadoghq.com>
Signed-off-by: Alexandre Rulleau <alexandre.rulleau@datadoghq.com>
Signed-off-by: Alexandre Rulleau <alexandre.rulleau@datadoghq.com>
Signed-off-by: Alexandre Rulleau <alexandre.rulleau@datadoghq.com>
Signed-off-by: Alexandre Rulleau <alexandre.rulleau@datadoghq.com>
@Leiyks Leiyks force-pushed the leiyks/add-laravel-login-and-signup-collection branch from e0b50b6 to 2cc1113 Compare January 20, 2025 09:46
Co-authored-by: Alexandre Choura <42672104+PROFeNoM@users.noreply.github.com>
@Leiyks Leiyks force-pushed the leiyks/add-laravel-login-and-signup-collection branch from 2cc1113 to 00c42f0 Compare January 20, 2025 10:29
@Leiyks Leiyks merged commit 7a0d358 into master Jan 20, 2025
704 of 745 checks passed
@Leiyks Leiyks deleted the leiyks/add-laravel-login-and-signup-collection branch January 20, 2025 13:23
@github-actions github-actions bot added this to the 1.7.0 milestone Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants