-
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
[SNMP] Componentize SNMP traps server #19789
Conversation
a2d5b93
to
37b1d91
Compare
f0c67c8
to
687c481
Compare
37b1d91
to
24d66ba
Compare
9931f51
to
7ac69d7
Compare
24d66ba
to
1e72390
Compare
Go Package Import DifferencesBaseline: e43ee12
|
7ac69d7
to
7664456
Compare
1e72390
to
8db2658
Compare
5189577
to
57b0cf2
Compare
57b0cf2
to
1fd94c6
Compare
Bloop Bleep... Dogbot HereRegression Detector ResultsRun ID: 4986105-063b-4ed5-b4ea-bb6d755bdc8c ExplanationA regression test is an integrated performance test for Because a target's optimization goal performance in each experiment will vary somewhat each time it is run, we can only estimate mean differences in optimization goal relative to the baseline target. We express these differences as a percentage change relative to the baseline target, denoted "Δ mean %". These estimates are made to a precision that balances accuracy and cost control. We represent this precision as a 90.00% confidence interval denoted "Δ mean % CI": there is a 90.00% chance that the true value of "Δ mean %" is in that interval. We decide whether a change in performance is a "regression" -- a change worth investigating further -- if both of the following two criteria are true:
The table below, if present, lists those experiments that have experienced a statistically significant change in mean optimization goal performance between baseline and comparison SHAs with 90.00% confidence OR have been detected as newly erratic. Negative values of "Δ mean %" mean that baseline is faster, whereas positive values of "Δ mean %" mean that comparison is faster. Results that do not exhibit more than a ±5.00% change in their mean optimization goal are discarded. An experiment is erratic if its coefficient of variation is greater than 0.1. The abbreviated table will be omitted if no interesting change is observed. No interesting changes in experiment optimization goals with confidence ≥ 90.00% and |Δ mean %| ≥ 5.00%. Fine details of change detection per experiment.
|
@@ -250,6 +250,47 @@ supported Datadog intakes. | |||
|
|||
|
|||
|
|||
## [comp/snmptraps](https://pkg.go.dev/github.com/DataDog/dd-agent-comp-experiments/comp/snmptraps) (Component Bundle) |
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.
Do these URLs need to be changed with this PR (not in dd-agent-comp-experiments)?
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 will fix them in another PR.
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.
@jmw51798 This readme is generated by inv lint-component
so changing it is outside the scope of this PR.
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.
Great PR, just minor comments.
@@ -250,6 +250,47 @@ supported Datadog intakes. | |||
|
|||
|
|||
|
|||
## [comp/snmptraps](https://pkg.go.dev/github.com/DataDog/dd-agent-comp-experiments/comp/snmptraps) (Component Bundle) |
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 will fix them in another PR.
comp/snmptraps/bundle.go
Outdated
package snmptraps | ||
|
||
import ( | ||
configimpl "github.com/DataDog/datadog-agent/comp/snmptraps/config/impl" |
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.
Ideally package impl
should be configimpl
to avoid alias. For example see this file
Note: Can be done in a later PR.
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.
Done.
In cases with mock implementations, would it make more sense for the real implementation and the mock implementation to be separate packages?
E.g. there are two formatter implementations, the JSONFormatter
and the DummyFormatter
- would it make more sense to have these under e.g. /formatter/json/
and /formatter/mock/
instead of a single formatter/formatterimpl
with both implementations in it?
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.
The current approach is to used build tags.
comp/snmptraps/bundle_test.go
Outdated
} | ||
|
||
func TestBundleDependencies(t *testing.T) { | ||
require.NoError(t, fx.ValidateApp( |
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 can simplify the test by using fxutil.TestBundle.
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.
Done.
"github.com/DataDog/datadog-agent/comp/core/log" | ||
// Module implements the formatter component. | ||
var Module = fxutil.Component( | ||
fx.Provide(NewJSONFormatter), |
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 NewJSONFormatter
be not exported?
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.
Done.
|
||
// Component is the component type. | ||
type Component interface { | ||
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.
Can you use Lifecycle hooks instead and not provide this method in the interface?
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.
Done.
comp/snmptraps/listener/component.go
Outdated
// Component is the component type. | ||
type Component interface { | ||
// Start starts the listener | ||
Start() error |
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 use Lifecycle hooks instead of Start
method?
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.
Done.
comp/snmptraps/server/impl/server.go
Outdated
} | ||
|
||
// Start starts the server listening | ||
func (s *Server) Start() error { |
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.
This method doesn't have to be exported, ie, 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.
Done. I actually removed this entirely - the server doesn't do anything now that the forwarder and listener use the lifecycle hooks, it now only exists as something you can inject that will make both the forwarder and the listener start running. But I also made the Start/Stop methods unexported on the forwarder and listener.
comp/snmptraps/server/impl/server.go
Outdated
} | ||
|
||
// Stop stops the TrapServer. | ||
func (s *Server) Stop() { |
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.
This method doesn't have to be exported, ie, stop.
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.
Done.
comp/snmptraps/status/impl/status.go
Outdated
// New creates a new manager | ||
func New() Manager { | ||
// New creates a new component | ||
func New() status.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.
Can you have a more explicit name and not export the function?
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.
Done.
a3ddef7
to
8d7b64c
Compare
@ogaca-dd I also modified this to depend on Do you think it would be worth providing the default |
return nil, err | ||
type dependencies struct { | ||
fx.In | ||
HostnameService hostname.Component `optional:"true"` |
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 should avoid optional
as there is no error when there is a missing dependency in hostname.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.
This is deliberate - tests don't need to include the hostname service unless they need to manipulate the hostname in some way. If a test is missing the dependency on hostname.Component, then newMockConfig
will use a default hostname.
mockSender.SetupAcceptAll() | ||
return mockSender, mockSender | ||
}), | ||
fx.Decorate(func(demux demultiplexer.Mock, s sender.Sender) demultiplexer.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.
Why using Decorate
instead of Provide
?
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.
demultiplexer.MockModule
provides both demultiplexer.Mock
and demultiplexer.Component
, so this has to be Decorate
or fx
will complain that there are multiple providers for demultiplexer.Component
.
I don't think |
What does this PR do?
This PR converts the SNMP traps server into a set of FX components.
The user-facing behavior should not change.
Motivation
We are trying to port all our services to the new component architecture.
This is part of NDMII-287.
Describe how to test/QA your changes
See internal instructions for QAing snmp traps. Existing QA should be fine, as the behavior of the traps listener is not expected to change.
Reviewer's Checklist
Triage
milestone is set.major_change
label if your change either has a major impact on the code base, is impacting multiple teams or is changing important well-established internals of the Agent. This label will be use during QA to make sure each team pay extra attention to the changed behavior. For any customer facing change use a releasenote.changelog/no-changelog
label has been applied.qa/skip-qa
label is not applied.team/..
label has been applied, indicating the team(s) that should QA this change.need-change/operator
andneed-change/helm
labels have been applied.k8s/<min-version>
label, indicating the lowest Kubernetes version compatible with this feature.