-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Removing start method in favor of on start hook in dogstatsd server component #22584
Removing start method in favor of on start hook in dogstatsd server component #22584
Conversation
…n-start-hook-in-dogstatsd-server-component
…n-start-hook-in-dogstatsd-server-component
…n-start-hook-in-dogstatsd-server-component
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.
Logic LGTM - mostly just comments around cleanup and small improvements
comp/aggregator/demultiplexer/demultiplexerimpl/demultiplexer.go
Outdated
Show resolved
Hide resolved
…k-in-dogstatsd-server-component' of github.com:DataDog/datadog-agent into AMLII-1408-remove-start-method-in-favor-of-on-start-hook-in-dogstatsd-server-component
Bloop Bleep... Dogbot HereRegression Detector ResultsRun ID: 408b6e6d-456b-4fac-b1e5-69a63239e8d0 Performance changes are noted in the perf column of each table:
No significant changes in experiment optimization goalsConfidence level: 90.00% There were no significant changes in experiment optimization goals at this confidence level and effect size tolerance.
|
perf | experiment | goal | Δ mean % | Δ mean % CI |
---|---|---|---|---|
➖ | file_to_blackhole | % cpu utilization | +1.06 | [-5.54, +7.67] |
Fine details of change detection per experiment
perf | experiment | goal | Δ mean % | Δ mean % CI |
---|---|---|---|---|
➖ | uds_dogstatsd_to_api_cpu | % cpu utilization | +1.47 | [+0.05, +2.90] |
➖ | file_to_blackhole | % cpu utilization | +1.06 | [-5.54, +7.67] |
➖ | process_agent_real_time_mode | memory utilization | +0.87 | [+0.82, +0.92] |
➖ | idle | memory utilization | +0.78 | [+0.73, +0.82] |
➖ | process_agent_standard_check | memory utilization | +0.15 | [+0.10, +0.21] |
➖ | trace_agent_json | ingress throughput | +0.03 | [+0.00, +0.05] |
➖ | trace_agent_msgpack | ingress throughput | +0.02 | [+0.00, +0.03] |
➖ | uds_dogstatsd_to_api | ingress throughput | +0.00 | [-0.00, +0.00] |
➖ | tcp_dd_logs_filter_exclude | ingress throughput | +0.00 | [-0.00, +0.00] |
➖ | process_agent_standard_check_with_stats | memory utilization | -0.13 | [-0.18, -0.08] |
➖ | file_tree | memory utilization | -0.15 | [-0.26, -0.05] |
➖ | tcp_syslog_to_blackhole | ingress throughput | -0.36 | [-0.41, -0.30] |
➖ | otel_to_otel_logs | ingress throughput | -0.92 | [-1.54, -0.30] |
Explanation
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
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 for agent-shared-components
🚂 MergeQueue Added to the queue. This build is going to start soon! (estimated merge in less than 48m) Use |
❌ MergeQueue Tests failed on this commit 5d242aa You should fix those tests and then re-add your pull request to the queue! Detailssome checks are failing: If you need support, contact us on Slack #ci-interfaces with those details! |
…n-start-hook-in-dogstatsd-server-component # Conflicts: # comp/aggregator/demultiplexer/demultiplexerimpl/demultiplexer.go
/merge |
🚂 MergeQueue Pull request added to the queue. This build is going to start soon! (estimated merge in less than 49m) Use |
❌ MergeQueue Tests failed on this commit 4ad76ca You should fix those tests and then re-add your pull request to the queue! Detailssome checks are failing: If you need support, contact us on Slack #ci-interfaces with those details! |
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.
You change the behavior when start returns an error. Is it expected?
@@ -22,7 +22,11 @@ import ( | |||
// FakeSamplerMockModule defines the fx options for FakeSamplerMock. | |||
func FakeSamplerMockModule() fxutil.Module { | |||
return fxutil.Component( | |||
fx.Provide(newFakeSamplerMock)) | |||
fx.Provide(newFakeSamplerMock), | |||
fx.Provide(func(demux demultiplexerComp.FakeSamplerMock) aggregator.Demultiplexer { |
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.
Can you explain why you need this as it is not common to provide non component type (except for Params
type).
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.
Since I replaced how a server is started, a new dogstatsd server no longer require a demultiplexer when started anymore. This causes some changes in the test files such as this function. Or any function that used to require the dogstatsdserver to include a demultiplexer.
Doing this provide a mock implementation instead of an actual component for those tests.
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.
Instead of using aggregator.Demultiplexer
can you use demultiplexer.Component
?
comp/dogstatsd/server/server.go
Outdated
s := newServerCompat(deps.Config, deps.Log, deps.Replay, deps.Debug, deps.Params.Serverless, deps.Demultiplexer) | ||
|
||
deps.Lc.Append(fx.Hook{ | ||
OnStart: s.start, |
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.
Here when start
returns an error the application logs the error whereas with your changes the application stops. Is this behavior expected?
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.
I changed the logic a bit here now so that when the server is running/stopped, it will log similarly instead. Good Catch! 🙇🏼♂️
…k-in-dogstatsd-server-component' of github.com:DataDog/datadog-agent into AMLII-1408-remove-start-method-in-favor-of-on-start-hook-in-dogstatsd-server-component
@@ -22,7 +22,11 @@ import ( | |||
// FakeSamplerMockModule defines the fx options for FakeSamplerMock. | |||
func FakeSamplerMockModule() fxutil.Module { | |||
return fxutil.Component( | |||
fx.Provide(newFakeSamplerMock)) | |||
fx.Provide(newFakeSamplerMock), | |||
fx.Provide(func(demux demultiplexerComp.FakeSamplerMock) aggregator.Demultiplexer { |
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.
Instead of using aggregator.Demultiplexer
can you use demultiplexer.Component
?
Serverless Benchmark Results
tl;dr
What is this benchmarking?The The benchmark is run using a large variety of lambda request payloads. In the charts below, there is one row for each event payload type. How do I interpret these charts?The charts below comes from The benchstat docs explain how to interpret these charts.
Benchmark stats
|
/merge |
🚂 MergeQueue Pull request added to the queue. There are 3 builds ahead! (estimated merge in less than 2h) Use |
…omponent (#22584) Removing start method in favor of on start hook in dogstatsd server component Co-authored-by: gh123man <[email protected]>
What does this PR do?
This PR injects the demultiplexer as a dependencies to Dogstatsdserver and remove the public Start method from the component interface
Motivation
Favor injecting the component over using a global
Describe how to test/QA your changes