-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Refactor stanzareceiver into a helper package (1/2) #2306
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2306 +/- ##
==========================================
- Coverage 90.63% 90.59% -0.04%
==========================================
Files 400 401 +1
Lines 19895 19899 +4
==========================================
- Hits 18031 18028 -3
- Misses 1411 1414 +3
- Partials 453 457 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Maybe place in internal/logreceiverhelper for now. |
Please resolve the conflicts. |
@djaglowski thanks a lot for working on this. You will unblock the rest of the logs issues. I know @pmm-sumo is also eager to work on some. |
…lector-contrib into stanza-helper
Of course. I'm eager to move it forward. Will have the followup PR asap. |
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
Will merge after the release. Something is wrong with CircleCI building the release, waiting for that. |
Signed-off-by: Bogdan Drutu <[email protected]>
**Link to tracking Issue:** This PR partially addresses the following issues: - Resolves: #2265 - Related: #2268, #2282. **Description:** The main idea here is to convert `stanzareceiver` into a helper package for building various other stanza-based receivers. Each of these other receivers will only vary by input operator. Functionality pulled out of `stanzareceiver` was moved into a new `filelogreceiver`. `stanzareceiver` should most likely be renamed and/or moved, but is left in its previous package for this initial PR. `stanzareceiver` defines an interface called `LogReceiverType` which each stanza-based receiver must implement and pass to `stanzareceiver.NewFactory(LogReceiverType) component.ReceiverFactory`. With this interface, each stanza-based receiver should only need a small amount of work to have a fully functional receiver. Support for parsing operations, emission from stanza's internal pipeline, and conversion to pdata format are all handled in the helper package so that these will be standardized across all the full set of stanza-based receivers. **Next Steps** Input operators are _not yet_ isolated to the top level of the configuration. The end goal is: ``` filelog: include: [ receiver/stanzareceiver/testdata/simple.log ] start_at: beginning operators: - type: regex_parser regex: '^(?P<time>\d{4}-\d{2}-\d{2}) (?P<sev>[A-Z]*) (?P<msg>.*)$' timestamp: parse_from: time layout: '%Y-%m-%d' severity: parse_from: sev ``` but the current state is still: ``` filelog: operators: - type: file_input include: [ receiver/stanzareceiver/testdata/simple.log ] start_at: beginning - type: regex_parser regex: '^(?P<time>\d{4}-\d{2}-\d{2}) (?P<sev>[A-Z]*) (?P<msg>.*)$' timestamp: parse_from: time layout: '%Y-%m-%d' severity: parse_from: sev ``` The primary requirement #2265 is to promote the input operator to the top level of the receiver config. This will be the focus of the next PR. This PR is mostly concerned with splitting up the package. The configuration changes might be a little messy so I wanted to address those separately. On the subject of configuration - the interface defined by `stanzareceiver` has a method `Decode(configmodels.Receiver) (pipeline.Config, error)` which is in my opinion much too loosely defined. Too much responsibility is delegated to each stanza-based receiver. The main reason this is left this way for now is that `stanza` operators do not currently use `mapstructure` for config unmarshaling. There is currently a workaround in place, but once stanza operators are migrated to `mapstructure`, more responsibility for unmarshaling should be extracted back into the helper package, and this interface method should end up a lot cleaner. I'm planning to look into this in the next PR. **Open questions** (which can be addressed in this PR or the next): - Should the helper package be completely standalone, or does it belong in `receivercreator` or similar? - If the helper package should be standalone, what should it be called? (probably not `stanzareceiver`) **Temporarily removed functionality** This functionality will be implemented in the near future. There is some design to do on how exactly this should work when used by multiple receivers: - Offsets database (tracked by #2287) - Plugins (tracked as item on #2264) **Testing:** Unit tests are roughly the same as before. A few cases were dropped because they no longer applied. Certainly more tests will be added as this pattern is solidified. Testbed scenario is unchanged and still passing: ``` > make run-tests ./runtests.sh === RUN TestLog10kDPS === RUN TestLog10kDPS/OTLP ... (abbreviated) === RUN TestLog10kDPS/Stanza ... (abbreviated) --- PASS: TestLog10kDPS (30.73s) --- PASS: TestLog10kDPS/OTLP (15.32s) --- PASS: TestLog10kDPS/Stanza (15.41s) PASS ok github.com/open-telemetry/opentelemetry-collector-contrib/testbed/tests_unstable_exe 31.406s # Test PerformanceResults Started: Mon, 08 Feb 2021 13:35:08 -0500 Test |Result|Duration|CPU Avg%|CPU Max%|RAM Avg MiB|RAM Max MiB|Sent Items|Received Items| ----------------------------------------|------|-------:|-------:|-------:|----------:|----------:|---------:|-------------:| Log10kDPS/OTLP |PASS | 15s| 19.9| 20.6| 39| 47| 149900| 149900| Log10kDPS/Stanza |PASS | 15s| 28.4| 29.3| 40| 48| 150000| 150000| Total duration: 31s ```
Link to tracking Issue:
This PR partially addresses the following issues:
Description:
The main idea here is to convert
stanzareceiver
into a helper package for building various other stanza-based receivers. Each of these other receivers will only vary by input operator. Functionality pulled out ofstanzareceiver
was moved into a newfilelogreceiver
.stanzareceiver
should most likely be renamed and/or moved, but is left in its previous package for this initial PR.stanzareceiver
defines an interface calledLogReceiverType
which each stanza-based receiver must implement and pass tostanzareceiver.NewFactory(LogReceiverType) component.ReceiverFactory
.With this interface, each stanza-based receiver should only need a small amount of work to have a fully functional receiver. Support for parsing operations, emission from stanza's internal pipeline, and conversion to pdata format are all handled in the helper package so that these will be standardized across all the full set of stanza-based receivers.
Next Steps
Input operators are not yet isolated to the top level of the configuration. The end goal is:
but the current state is still:
The primary requirement #2265 is to promote the input operator to the top level of the receiver config. This will be the focus of the next PR. This PR is mostly concerned with splitting up the package. The configuration changes might be a little messy so I wanted to address those separately.
On the subject of configuration - the interface defined by
stanzareceiver
has a methodDecode(configmodels.Receiver) (pipeline.Config, error)
which is in my opinion much too loosely defined. Too much responsibility is delegated to each stanza-based receiver. The main reason this is left this way for now is thatstanza
operators do not currently usemapstructure
for config unmarshaling. There is currently a workaround in place, but once stanza operators are migrated tomapstructure
, more responsibility for unmarshaling should be extracted back into the helper package, and this interface method should end up a lot cleaner. I'm planning to look into this in the next PR.Open questions (which can be addressed in this PR or the next):
receivercreator
or similar?stanzareceiver
)Temporarily removed functionality
This functionality will be implemented in the near future. There is some design to do on how exactly this should work when used by multiple receivers:
Testing:
Unit tests are roughly the same as before. A few cases were dropped because they no longer applied. Certainly more tests will be added as this pattern is solidified.
Testbed scenario is unchanged and still passing: