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

Make propagation happen on AsyncResponse using CFX JAX-RS #6313

Merged
merged 2 commits into from
Dec 8, 2023

Conversation

amarziali
Copy link
Collaborator

@amarziali amarziali commented Dec 4, 2023

What Does This Do

When async response is suspended, on CXF jax-rs, the subsequent invoke methods (response resume, exception mappers) are not propagating at least the root servlet span.
This PR is fixing this

Motivation

Additional Notes

Jira ticket: APMS-10975

@amarziali amarziali requested a review from a team as a code owner December 4, 2023 21:07
@amarziali amarziali added inst: jax-rs JAX-RS instrumentation type: bug labels Dec 4, 2023
@amarziali amarziali force-pushed the andrea.marziali/cxf-continuations branch from 31db51b to e979df0 Compare December 4, 2023 21:37
@pr-commenter
Copy link

pr-commenter bot commented Dec 4, 2023

Benchmarks

Startup

Parameters

Baseline Candidate
baseline_or_candidate baseline candidate
git_branch master andrea.marziali/cxf-continuations
git_commit_date 1701949795 1701951238
git_commit_sha 21e5b46 a6bef34
release_version 1.26.0-SNAPSHOT~21e5b464db 1.26.0-SNAPSHOT~a6bef34154
See matching parameters
Baseline Candidate
application insecure-bank insecure-bank
ci_job_date 1701953855 1701953855
ci_job_id 384412169 384412169
ci_pipeline_id 24624993 24624993
cpu_model Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz
module Agent Agent
parent None None
variant iast iast

Summary

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

Startup time reports for insecure-bank
gantt
    title insecure-bank - global startup overhead: candidate=1.26.0-SNAPSHOT~a6bef34154, baseline=1.26.0-SNAPSHOT~21e5b464db

    dateFormat X
    axisFormat %s
section tracing
Agent [baseline] (1.047 s) : 0, 1047018
Total [baseline] (8.696 s) : 0, 8695981
Agent [candidate] (1.049 s) : 0, 1049064
Total [candidate] (8.763 s) : 0, 8763368
section iast
Agent [baseline] (1.158 s) : 0, 1157578
Total [baseline] (9.248 s) : 0, 9248418
Agent [candidate] (1.17 s) : 0, 1169936
Total [candidate] (9.288 s) : 0, 9288261
section iast_TELEMETRY_OFF
Agent [baseline] (1.157 s) : 0, 1157141
Total [baseline] (9.257 s) : 0, 9257163
Agent [candidate] (1.152 s) : 0, 1152172
Total [candidate] (9.279 s) : 0, 9279313
Loading
  • baseline results
Module Variant Duration Δ tracing
Agent tracing 1.047 s -
Agent iast 1.158 s 110.56 ms (10.6%)
Agent iast_TELEMETRY_OFF 1.157 s 110.123 ms (10.5%)
Total tracing 8.696 s -
Total iast 9.248 s 552.437 ms (6.4%)
Total iast_TELEMETRY_OFF 9.257 s 561.182 ms (6.5%)
  • candidate results
Module Variant Duration Δ tracing
Agent tracing 1.049 s -
Agent iast 1.17 s 120.872 ms (11.5%)
Agent iast_TELEMETRY_OFF 1.152 s 103.107 ms (9.8%)
Total tracing 8.763 s -
Total iast 9.288 s 524.893 ms (6.0%)
Total iast_TELEMETRY_OFF 9.279 s 515.945 ms (5.9%)
gantt
    title insecure-bank - break down per module: candidate=1.26.0-SNAPSHOT~a6bef34154, baseline=1.26.0-SNAPSHOT~21e5b464db

    dateFormat X
    axisFormat %s
section tracing
BytebuddyAgent [baseline] (648.386 ms) : 0, 648386
BytebuddyAgent [candidate] (648.97 ms) : 0, 648970
GlobalTracer [baseline] (308.015 ms) : 0, 308015
GlobalTracer [candidate] (308.854 ms) : 0, 308854
AppSec [baseline] (48.408 ms) : 0, 48408
AppSec [candidate] (48.897 ms) : 0, 48897
Remote Config [baseline] (674.112 µs) : 0, 674
Remote Config [candidate] (677.712 µs) : 0, 678
Telemetry [baseline] (7.138 ms) : 0, 7138
Telemetry [candidate] (7.147 ms) : 0, 7147
section iast
BytebuddyAgent [baseline] (765.091 ms) : 0, 765091
BytebuddyAgent [candidate] (772.153 ms) : 0, 772153
GlobalTracer [baseline] (283.967 ms) : 0, 283967
GlobalTracer [candidate] (287.185 ms) : 0, 287185
AppSec [baseline] (46.278 ms) : 0, 46278
AppSec [candidate] (46.794 ms) : 0, 46794
IAST [baseline] (20.787 ms) : 0, 20787
IAST [candidate] (20.435 ms) : 0, 20435
Remote Config [baseline] (611.624 µs) : 0, 612
Remote Config [candidate] (610.178 µs) : 0, 610
Telemetry [baseline] (6.579 ms) : 0, 6579
Telemetry [candidate] (8.127 ms) : 0, 8127
section iast_TELEMETRY_OFF
BytebuddyAgent [baseline] (763.718 ms) : 0, 763718
BytebuddyAgent [candidate] (758.395 ms) : 0, 758395
GlobalTracer [baseline] (285.159 ms) : 0, 285159
GlobalTracer [candidate] (285.666 ms) : 0, 285666
AppSec [baseline] (46.215 ms) : 0, 46215
AppSec [candidate] (46.968 ms) : 0, 46968
IAST [baseline] (18.964 ms) : 0, 18964
IAST [candidate] (19.811 ms) : 0, 19811
Remote Config [baseline] (633.683 µs) : 0, 634
Remote Config [candidate] (610.409 µs) : 0, 610
Telemetry [baseline] (7.932 ms) : 0, 7932
Telemetry [candidate] (6.478 ms) : 0, 6478
Loading
Startup time reports for petclinic
gantt
    title petclinic - global startup overhead: candidate=1.26.0-SNAPSHOT~a6bef34154, baseline=1.26.0-SNAPSHOT~21e5b464db

    dateFormat X
    axisFormat %s
section tracing
Agent [baseline] (1.043 s) : 0, 1043089
Total [baseline] (9.376 s) : 0, 9376097
Agent [candidate] (1.042 s) : 0, 1041503
Total [candidate] (9.352 s) : 0, 9352207
section appsec
Agent [baseline] (1.137 s) : 0, 1137001
Total [baseline] (9.446 s) : 0, 9445826
Agent [candidate] (1.132 s) : 0, 1132035
Total [candidate] (9.387 s) : 0, 9386814
section iast
Agent [baseline] (1.157 s) : 0, 1157245
Total [baseline] (9.522 s) : 0, 9521835
Agent [candidate] (1.16 s) : 0, 1160417
Total [candidate] (9.565 s) : 0, 9564885
section profiling
Agent [baseline] (1.238 s) : 0, 1237782
Total [baseline] (9.589 s) : 0, 9588586
Agent [candidate] (1.228 s) : 0, 1227974
Total [candidate] (9.625 s) : 0, 9624938
Loading
  • baseline results
Module Variant Duration Δ tracing
Agent tracing 1.043 s -
Agent appsec 1.137 s 93.912 ms (9.0%)
Agent iast 1.157 s 114.156 ms (10.9%)
Agent profiling 1.238 s 194.693 ms (18.7%)
Total tracing 9.376 s -
Total appsec 9.446 s 69.729 ms (0.7%)
Total iast 9.522 s 145.738 ms (1.6%)
Total profiling 9.589 s 212.489 ms (2.3%)
  • candidate results
Module Variant Duration Δ tracing
Agent tracing 1.042 s -
Agent appsec 1.132 s 90.532 ms (8.7%)
Agent iast 1.16 s 118.914 ms (11.4%)
Agent profiling 1.228 s 186.471 ms (17.9%)
Total tracing 9.352 s -
Total appsec 9.387 s 34.607 ms (0.4%)
Total iast 9.565 s 212.678 ms (2.3%)
Total profiling 9.625 s 272.731 ms (2.9%)
gantt
    title petclinic - break down per module: candidate=1.26.0-SNAPSHOT~a6bef34154, baseline=1.26.0-SNAPSHOT~21e5b464db

    dateFormat X
    axisFormat %s
section tracing
BytebuddyAgent [baseline] (645.18 ms) : 0, 645180
BytebuddyAgent [candidate] (644.669 ms) : 0, 644669
GlobalTracer [baseline] (307.516 ms) : 0, 307516
GlobalTracer [candidate] (306.35 ms) : 0, 306350
AppSec [baseline] (48.351 ms) : 0, 48351
AppSec [candidate] (48.452 ms) : 0, 48452
Remote Config [baseline] (676.818 µs) : 0, 677
Remote Config [candidate] (677.667 µs) : 0, 678
Telemetry [baseline] (7.211 ms) : 0, 7211
Telemetry [candidate] (7.162 ms) : 0, 7162
section appsec
BytebuddyAgent [baseline] (648.299 ms) : 0, 648299
BytebuddyAgent [candidate] (646.035 ms) : 0, 646035
GlobalTracer [baseline] (307.793 ms) : 0, 307793
GlobalTracer [candidate] (307.013 ms) : 0, 307013
AppSec [baseline] (137.575 ms) : 0, 137575
AppSec [candidate] (136.771 ms) : 0, 136771
Remote Config [baseline] (642.969 µs) : 0, 643
Remote Config [candidate] (644.399 µs) : 0, 644
Telemetry [baseline] (8.224 ms) : 0, 8224
Telemetry [candidate] (7.419 ms) : 0, 7419
section iast
BytebuddyAgent [baseline] (765.004 ms) : 0, 765004
BytebuddyAgent [candidate] (766.587 ms) : 0, 766587
GlobalTracer [baseline] (284.395 ms) : 0, 284395
GlobalTracer [candidate] (285.15 ms) : 0, 285150
AppSec [baseline] (46.275 ms) : 0, 46275
AppSec [candidate] (46.38 ms) : 0, 46380
Remote Config [baseline] (589.498 µs) : 0, 589
Remote Config [candidate] (602.936 µs) : 0, 603
Telemetry [baseline] (8.014 ms) : 0, 8014
Telemetry [candidate] (8.813 ms) : 0, 8813
IAST [baseline] (18.776 ms) : 0, 18776
IAST [candidate] (18.691 ms) : 0, 18691
section profiling
ProfilingAgent [baseline] (88.497 ms) : 0, 88497
ProfilingAgent [candidate] (88.118 ms) : 0, 88118
BytebuddyAgent [baseline] (659.287 ms) : 0, 659287
BytebuddyAgent [candidate] (654.079 ms) : 0, 654079
GlobalTracer [baseline] (378.907 ms) : 0, 378907
GlobalTracer [candidate] (375.749 ms) : 0, 375749
AppSec [baseline] (48.622 ms) : 0, 48622
AppSec [candidate] (48.16 ms) : 0, 48160
Remote Config [baseline] (699.592 µs) : 0, 700
Remote Config [candidate] (693.375 µs) : 0, 693
Telemetry [baseline] (7.436 ms) : 0, 7436
Telemetry [candidate] (7.312 ms) : 0, 7312
Profiling [baseline] (88.522 ms) : 0, 88522
Profiling [candidate] (88.146 ms) : 0, 88146
Loading

Load

Parameters

Baseline Candidate
baseline_or_candidate baseline candidate
end_time 2023-12-07T12:36:51 2023-12-07T12:53:24
git_branch master andrea.marziali/cxf-continuations
git_commit_date 1701949795 1701951238
git_commit_sha 21e5b46 a6bef34
release_version 1.26.0-SNAPSHOT~21e5b464db 1.26.0-SNAPSHOT~a6bef34154
start_time 2023-12-07T12:36:38 2023-12-07T12:53:11
See matching parameters
Baseline Candidate
application insecure-bank insecure-bank
ci_job_date 1701953855 1701953855
ci_job_id 384412169 384412169
ci_pipeline_id 24624993 24624993
cpu_model Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz
variant iast iast

Summary

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

Request duration reports for insecure-bank
gantt
    title insecure-bank - request duration [CI 0.99] : candidate=1.26.0-SNAPSHOT~a6bef34154, baseline=1.26.0-SNAPSHOT~21e5b464db
    dateFormat X
    axisFormat %s
section baseline
no_agent (371.024 µs) : 350, 392
.   : milestone, 371,
iast (473.013 µs) : 452, 494
.   : milestone, 473,
iast_FULL (533.342 µs) : 513, 554
.   : milestone, 533,
iast_INACTIVE (449.597 µs) : 428, 471
.   : milestone, 450,
iast_TELEMETRY_OFF (468.297 µs) : 448, 489
.   : milestone, 468,
tracing (437.279 µs) : 417, 457
.   : milestone, 437,
section candidate
no_agent (361.078 µs) : 341, 381
.   : milestone, 361,
iast (467.537 µs) : 447, 488
.   : milestone, 468,
iast_FULL (537.777 µs) : 517, 558
.   : milestone, 538,
iast_INACTIVE (446.037 µs) : 426, 467
.   : milestone, 446,
iast_TELEMETRY_OFF (465.685 µs) : 445, 486
.   : milestone, 466,
tracing (431.818 µs) : 412, 452
.   : milestone, 432,
Loading
  • baseline results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 371.024 µs [349.555 µs, 392.494 µs] -
iast 473.013 µs [452.438 µs, 493.589 µs] 101.989 µs (27.5%)
iast_FULL 533.342 µs [512.887 µs, 553.796 µs] 162.317 µs (43.7%)
iast_INACTIVE 449.597 µs [428.478 µs, 470.716 µs] 78.573 µs (21.2%)
iast_TELEMETRY_OFF 468.297 µs [447.575 µs, 489.02 µs] 97.273 µs (26.2%)
tracing 437.279 µs [417.185 µs, 457.373 µs] 66.255 µs (17.9%)
  • candidate results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 361.078 µs [340.987 µs, 381.168 µs] -
iast 467.537 µs [447.022 µs, 488.052 µs] 106.459 µs (29.5%)
iast_FULL 537.777 µs [517.296 µs, 558.259 µs] 176.7 µs (48.9%)
iast_INACTIVE 446.037 µs [425.569 µs, 466.505 µs] 84.959 µs (23.5%)
iast_TELEMETRY_OFF 465.685 µs [444.958 µs, 486.413 µs] 104.608 µs (29.0%)
tracing 431.818 µs [411.571 µs, 452.065 µs] 70.74 µs (19.6%)
Request duration reports for petclinic
gantt
    title petclinic - request duration [CI 0.99] : candidate=1.26.0-SNAPSHOT~a6bef34154, baseline=1.26.0-SNAPSHOT~21e5b464db
    dateFormat X
    axisFormat %s
section baseline
no_agent (1.342 ms) : 1323, 1361
.   : milestone, 1342,
appsec (1.767 ms) : 1742, 1792
.   : milestone, 1767,
iast (1.533 ms) : 1509, 1557
.   : milestone, 1533,
profiling (1.497 ms) : 1472, 1523
.   : milestone, 1497,
tracing (1.492 ms) : 1468, 1517
.   : milestone, 1492,
section candidate
no_agent (1.344 ms) : 1325, 1363
.   : milestone, 1344,
appsec (1.734 ms) : 1708, 1760
.   : milestone, 1734,
iast (1.516 ms) : 1492, 1540
.   : milestone, 1516,
profiling (1.504 ms) : 1479, 1529
.   : milestone, 1504,
tracing (1.498 ms) : 1473, 1523
.   : milestone, 1498,
Loading
  • baseline results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 1.342 ms [1.323 ms, 1.361 ms] -
appsec 1.767 ms [1.742 ms, 1.792 ms] 425.422 µs (31.7%)
iast 1.533 ms [1.509 ms, 1.557 ms] 190.788 µs (14.2%)
profiling 1.497 ms [1.472 ms, 1.523 ms] 155.547 µs (11.6%)
tracing 1.492 ms [1.468 ms, 1.517 ms] 150.465 µs (11.2%)
  • candidate results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 1.344 ms [1.325 ms, 1.363 ms] -
appsec 1.734 ms [1.708 ms, 1.76 ms] 390.343 µs (29.1%)
iast 1.516 ms [1.492 ms, 1.54 ms] 172.311 µs (12.8%)
profiling 1.504 ms [1.479 ms, 1.529 ms] 160.276 µs (11.9%)
tracing 1.498 ms [1.473 ms, 1.523 ms] 154.586 µs (11.5%)

@amarziali amarziali force-pushed the andrea.marziali/cxf-continuations branch from e979df0 to 8c7aa48 Compare December 4, 2023 21:49
Copy link
Contributor

@PerfectSlayer PerfectSlayer left a comment

Choose a reason for hiding this comment

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

Hey 👋
Just some minor comments

@PerfectSlayer PerfectSlayer changed the title cxf: make propagation happen on AsyncResponse Make propagation happen on AsyncResponse using CFX JAX-RS Dec 6, 2023
@amarziali amarziali force-pushed the andrea.marziali/cxf-continuations branch from 8c7aa48 to e4112f9 Compare December 6, 2023 12:32
@amarziali
Copy link
Collaborator Author

Hey 👋 Just some minor comments

Thanks for the review I applied the suggestions. There are some muzzle failures I should have also resolved (linked to missing extra dependencies). The customer already tested a CI build on real conditions and seems work as expected. Could you please have another look? thanks

@ejhnsn
Copy link

ejhnsn commented Dec 6, 2023

The customer already tested a CI build on real conditions and seems work as expected.

I am said customer and am happy to help with any additional testing if you'd like.

@amarziali amarziali force-pushed the andrea.marziali/cxf-continuations branch 2 times, most recently from 90336da to a6bef34 Compare December 7, 2023 12:14
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

// Probably one day we should put it in the bootstrap if used in a lot of places
Copy link
Contributor

Choose a reason for hiding this comment

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

As part of refactoring the build I'd like to make it easier to share utilities based on libraries like the servlet-api. And ideally do this in a way such that the right utility is chosen at advice insertion time, which would reduce the need for method handles.

Copy link
Contributor

@mcculls mcculls left a comment

Choose a reason for hiding this comment

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

The use of method handles here to handle javax.servet vs jakarta.servlet deployments is acceptable for the short-term. In the long-term I hope to make improvements to the build which would allow us to remove these.

@amarziali amarziali force-pushed the andrea.marziali/cxf-continuations branch from a6bef34 to fba3fe1 Compare December 8, 2023 13:39
…ace/instrumentation/cxf/ServletHelper.java

Co-authored-by: Stuart McCulloch <stuart.mcculloch@datadoghq.com>
@amarziali amarziali enabled auto-merge (squash) December 8, 2023 13:39
@amarziali amarziali merged commit 2a7762c into master Dec 8, 2023
68 of 72 checks passed
@amarziali amarziali deleted the andrea.marziali/cxf-continuations branch December 8, 2023 14:11
@github-actions github-actions bot added this to the 1.26.0 milestone Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inst: jax-rs JAX-RS instrumentation type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants