Skip to content
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

[pipelines] Change test to not reuse same processor twice in one pipeline #6540

Merged

Conversation

djaglowski
Copy link
Member

There is documentation here that explains how a single processor can be specified in multiple pipelines, with the understanding that each pipeline will get its own instance. However, there is nothing specifically stated about the case where a single processor is specified multiple times in a single pipeline.

This PR is as much a question as a proposal. Is there an explicit intention to support this use case, or should we clarify and disallow it?

@codecov
Copy link

codecov bot commented Nov 14, 2022

Codecov Report

Base: 90.98% // Head: 90.99% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (3a5f156) compared to base (99f4496).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6540   +/-   ##
=======================================
  Coverage   90.98%   90.99%           
=======================================
  Files         241      241           
  Lines       14005    14011    +6     
=======================================
+ Hits        12743    12749    +6     
  Misses       1013     1013           
  Partials      249      249           
Impacted Files Coverage Δ
service/config.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@djaglowski djaglowski force-pushed the processor-duplication-per-pipeline branch from 241a61e to 8c534d6 Compare November 14, 2022 17:55
@djaglowski djaglowski marked this pull request as ready for review November 14, 2022 17:55
@djaglowski djaglowski requested review from a team and Aneurysm9 November 14, 2022 17:55
@djaglowski
Copy link
Member Author

cc: @bogdandrutu, @tigrannajaryan

@tigrannajaryan
Copy link
Member

I think there can be valid reasons to use the same processor type twice in one pipeline (each processor having a different config). I am not sure though if there are valid use cases for using the same processor type with the same config twice.

@bogdandrutu
Copy link
Member

This PR is as much a question as a proposal. Is there an explicit intention to support this use case, or should we clarify and disallow it?

I don't necessary know about an use-case, so we can disallow this for the moment. If we allow it we should document that a new instance is created.

@djaglowski
Copy link
Member Author

Ok, sounds like we are in agreement that this is not a supported configuration. I've added a note to the documentation.

Should I also explicitly disallow this in code?

docs/design.md Outdated Show resolved Hide resolved
@bogdandrutu bogdandrutu reopened this Nov 16, 2022
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the docs change (I don't see much value in supporting this) and I think we should fix it in the code too

@djaglowski djaglowski force-pushed the processor-duplication-per-pipeline branch from 4a0b2b0 to 3a5f156 Compare November 17, 2022 15:04
@djaglowski
Copy link
Member Author

I've added a check in code. Should we consider this a breaking change or just a bug fix?

@bogdandrutu bogdandrutu merged commit c205595 into open-telemetry:main Nov 17, 2022
@djaglowski djaglowski deleted the processor-duplication-per-pipeline branch November 17, 2022 16:31
@adam-kiss-sg
Copy link

I just run into this error message. My use-case is using multiple batch processor in the same pipeline. From the batch processor docs:

The batch processor should be defined in the pipeline after the memory_limiter as well as any sampling processors.

So based on that i have something like:

processors: [memory_limiter, batch, filter, batch]

where filter filters out a lot of metrics.

Am i using the batch processor wrong? What should be the best practice here?

@mx-psi
Copy link
Member

mx-psi commented Nov 21, 2022

@adam-kiss-sg

What should be the best practice here?

IMO that's acceptable use of the batch processor. You just need to define two instances of the processor:

processors:
  batch:
    # maybe some configuration here
  batch/2:
    # maybe some configuration here

#  Omitted rest of the config

service:
  pipelines:
    traces:
      receiver: [otlp]
      processors: [memory_limiter, batch, filter, batch/2]
      exporters: [otlp]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants