Add processor helper. In the future add metrics and tracing #1359
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bogdandrutu
requested review from
dmitryax,
flands,
james-bebbington,
owais,
pjanotti,
rghetia and
tigrannajaryan
as code owners
james-bebbington
approved these changes
Jul 15, 2020
| @@ -28,6 +28,8 @@ const ( | |||
| typeStr = "filter" | |||
| ) | |||
|
|
|||
| var processorCapabilities = component.ProcessorCapabilities{MutatesConsumedData: false} | |||
|
|
|||
| // NewFactory returns a new factory for the Filter processor. | |||
| func NewFactory() component.ProcessorFactory { | |||
| return processorhelper.NewFactory( | |||
There was a problem hiding this comment.
Instead of:
processorhelper.WithMetrics(createMetricsProcessor)Would it make sense to consider working towards:
processorhelper.WithMetrics(filterMetrics, processorhelper.WithCapabilities(processorCapabilities))And eventually remove the need for processorhelper/processor.go.
Not sure if that's feasible given the rest of the startup code & need for backwards compatibilty.
There was a problem hiding this comment.
I will think about, but seems hard to cover all the cases:
- Somewhere you need a new instance of the underlying processor that is wrapped so there needs to be a function that does what the current create does before calling into the new helper.
- Capabilities may differ based on the config or params (things that are passed during create method). I know we don't do that now but it is a possibility.
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package processorhelper |
There was a problem hiding this comment.
How about moving this up a level and making this just e.g. processor.NewTraceProcessor to signify this is the first class way that components should be created?
There was a problem hiding this comment.
I can do that in a follow-up.
bogdandrutu
force-pushed
the
processorhelper
branch
4 times, most recently
from
6e20722
to
4bb1f60
Compare
bogdandrutu
force-pushed
the
processorhelper
branch
from
4bb1f60
to
1d015b5
Compare
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu
force-pushed
the
processorhelper
branch
from
1d015b5
to
7a36529
Compare
Codecov Report
@@ Coverage Diff @@
## master #1359 +/- ##
==========================================
+ Coverage 90.11% 90.28% +0.16%
==========================================
Files 217 218 +1
Lines 15242 15299 +57
==========================================
+ Hits 13736 13813 +77
+ Misses 1096 1076 -20
Partials 410 410
Continue to review full report at Codecov.
|
MovieStoreGuy
pushed a commit
to atlassian-forks/opentelemetry-collector
that referenced
this pull request
Nov 11, 2021
And update .gitignore, so the built binary does not show up in git status.
hughesjj
pushed a commit
to hughesjj/opentelemetry-collector
that referenced
this pull request
Apr 27, 2023
* Update smart agent deps * Update smart agent bundle to 5.19.1 * Update changelog
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.