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

Introduce OTLP receiver configuration flags #3710

Merged
merged 7 commits into from
May 30, 2022

Conversation

yurishkuro
Copy link
Member

@yurishkuro yurishkuro commented May 27, 2022

Part of #3625.

  • Add CLI flags to control OTLP receiver.
  • Move collector flags to app/flags for cleaner reuse (specifically from app/handlers)
  • Refactor flags to allow reuse of HTTP and GRPC options (Zipkin options are still handled separately)
  • Not all configuration from OTLP Receiver is supported, only the flags that we already had in the other HTTP and gRPC servers.

@codecov
Copy link

codecov bot commented May 27, 2022

Codecov Report

Merging #3710 (068fe18) into main (22e9143) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3710      +/-   ##
==========================================
+ Coverage   97.40%   97.49%   +0.09%     
==========================================
  Files         269      269              
  Lines       15856    15934      +78     
==========================================
+ Hits        15444    15535      +91     
+ Misses        326      315      -11     
+ Partials       86       84       -2     
Impacted Files Coverage Δ
cmd/collector/app/span_handler_builder.go 100.00% <ø> (ø)
cmd/collector/app/collector.go 87.70% <100.00%> (+3.20%) ⬆️
cmd/collector/app/flags/flags.go 100.00% <100.00%> (ø)
cmd/collector/app/handler/otlp_receiver.go 100.00% <100.00%> (ø)
cmd/collector/app/options.go 100.00% <100.00%> (ø)
pkg/config/tlscfg/cert_watcher.go 94.73% <0.00%> (+2.10%) ⬆️
cmd/query/app/static_handler.go 97.60% <0.00%> (+3.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22e9143...068fe18. Read the comment docs.

@yurishkuro yurishkuro force-pushed the otlp-receiver-config branch 2 times, most recently from 63b3352 to 4ffcf5d Compare May 29, 2022 20:52
// See the License for the specific language governing permissions and
// limitations under the License.

package flags
Copy link
Member Author

Choose a reason for hiding this comment

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

this was a move from app/flags.go, but unfortunately git does not recognize it as move due to many changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

This commit shows the actual diffs cb6a8c9

@yurishkuro yurishkuro force-pushed the otlp-receiver-config branch from 4ffcf5d to 92695ba Compare May 29, 2022 21:07
@yurishkuro yurishkuro changed the title WIP: OTLP receiver configuration Introduce OTLP receiver configuration flags May 30, 2022
@yurishkuro yurishkuro marked this pull request as ready for review May 30, 2022 01:13
@yurishkuro yurishkuro requested a review from a team as a code owner May 30, 2022 01:13
@yurishkuro yurishkuro requested a review from pavolloffay May 30, 2022 01:13
@yurishkuro
Copy link
Member Author

@albertteoh would you like to review this one too?

albertteoh
albertteoh previously approved these changes May 30, 2022
Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 113 to 114
createTracesReceiver := func(_ context.Context, _ component.ReceiverCreateSettings,
_ config.Receiver, _ consumer.Traces) (component.TracesReceiver, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Since none of the params are used, they can be removed altogether; it's not necessary to add the blank identifiers. Same with above newTraces := func(...).

Suggested change
createTracesReceiver := func(_ context.Context, _ component.ReceiverCreateSettings,
_ config.Receiver, _ consumer.Traces) (component.TracesReceiver, error) {
createTracesReceiver := func(context.Context, component.ReceiverCreateSettings,
config.Receiver, consumer.Traces) (component.TracesReceiver, error) {

Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
@yurishkuro
Copy link
Member Author

Thanks for the review, @albertteoh

@yurishkuro yurishkuro merged commit 6e9303c into jaegertracing:main May 30, 2022
@yurishkuro yurishkuro deleted the otlp-receiver-config branch May 30, 2022 18:45
albertteoh pushed a commit to albertteoh/jaeger that referenced this pull request Jul 13, 2022
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.

2 participants