-
Notifications
You must be signed in to change notification settings - Fork 10
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
Handle case where the applictaion is stopped without other telemetry sent #743
Conversation
aa2a1eb
to
afec95c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but is there any reasonable way to test this either in libdatadog or in php?
BenchmarksComparisonBenchmark execution time: 2024-11-15 21:59:55 Comparing candidate commit a03f96d in PR branch Found 2 performance improvements and 2 performance regressions! Performance is the same for 47 metrics, 2 unstable metrics. scenario:credit_card/is_card_number_no_luhn/378282246310005
scenario:normalization/normalize_service/normalize_service/Data🐨dog🐶 繋がっ⛰てて
CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
BaselineOmitted due to size. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #743 +/- ##
==========================================
+ Coverage 70.54% 70.55% +0.01%
==========================================
Files 296 296
Lines 43277 43281 +4
==========================================
+ Hits 30530 30539 +9
+ Misses 12747 12742 -5
|
afec95c
to
ae035cb
Compare
…sent This lead down to a path where the Stop action was actually enqueued without being ever causing a removal of the Application instances. Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
ae035cb
to
5495314
Compare
@ekump Some scernarios are indeed hard to test with end-to-end coverage, as testing that would need to setup a webserver onto which to fire multiple requests from the php side. And then it doesn't really have a visible side effect (except memory increase). Testing the sidecar with actual #[tokio::test] is also not trivial either, as a lot of the handlers do tokio::spawn(), and there's no synchronization point on it. The sidecar_server as a whole is quite a bit unwieldy and possibly could benefit from refactors to make it more testable. Which probably isn't easy either. |
This lead down to a path where the Stop action was actually enqueued without being ever causing a removal of the Application instances.