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

Add stdin receiver #1849

Closed
wants to merge 5 commits into from
Closed

Add stdin receiver #1849

wants to merge 5 commits into from

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Dec 15, 2020

Description:
Adding a stdin receiver.
Allows to listen to stdin from one or more receivers, to process data.
Useful for simple activities of data collection, especially around logs, and for debugging purposes.

Testing:
Unit tests.

Documentation:
Just a testdata folder and a README.

@atoulme atoulme requested a review from a team December 15, 2020 22:31
@codecov
Copy link

codecov bot commented Dec 15, 2020

Codecov Report

Merging #1849 (ae68d80) into master (edb91c3) will increase coverage by 0.01%.
The diff coverage is 96.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1849      +/-   ##
==========================================
+ Coverage   89.83%   89.84%   +0.01%     
==========================================
  Files         376      378       +2     
  Lines       18211    18265      +54     
==========================================
+ Hits        16359    16411      +52     
- Misses       1388     1389       +1     
- Partials      464      465       +1     
Flag Coverage Δ
integration 69.77% <0.00%> (-0.05%) ⬇️
unit 88.55% <96.29%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
receiver/stdinreceiver/receiver.go 95.12% <95.12%> (ø)
cmd/otelcontribcol/components.go 86.30% <100.00%> (+0.19%) ⬆️
receiver/stdinreceiver/factory.go 100.00% <100.00%> (ø)

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 edb91c3...ae68d80. Read the comment docs.

receiver/stdinreceiver/go.mod Outdated Show resolved Hide resolved
ctx = obsreport.StartLogsReceiveOp(ctx, r.config.Name(), transport)
ld := pdata.NewLogs()
rl := pdata.NewResourceLogs()
ld.ResourceLogs().Append(rl)
Copy link
Member

Choose a reason for hiding this comment

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

What is the plan on how Resource attributes get populated? If expected to happen further down the processing pipeline, please document in README

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No plan that I can think of. What would you place in the Resource right now?

Copy link
Member

Choose a reason for hiding this comment

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

I would set service.name and service.version for sure with a configurable values and probably host and process values. I would add to the README instructing users to wire in the k8s_tagger and/or resourcedetection processors to provide further population of values where applicable.

receiver/stdinreceiver/receiver.go Show resolved Hide resolved
@gramidt
Copy link
Member

gramidt commented Dec 28, 2020

Good work on this! Excited to test it out! Out of curiosity, was there a particular reason this wasn't split into multiple PRs? (First for structure, second for the implementation, and third for adding to the binary.)

@atoulme
Copy link
Contributor Author

atoulme commented Dec 29, 2020

No particular reason - I just wrote it in one go. Is there a guideline to do this in 3 PRs?

@gramidt
Copy link
Member

gramidt commented Dec 29, 2020

Totally understand, @atoulme! :)

There is a rough guideline when adding new components that can be found in the contributing docs.

  • First PR should include the overall structure of the new component:
    • Readme, configuration, and factory implementation usually using the helper factory structs.
    • This PR is usually trivial to review, so the size limit does not apply to it.
  • Second PR should include the concrete implementation of the component.
    If the size of this PR is larger than the recommended size consider splitting it in multiple PRs.
  • Last PR should enable the new component and add it to the otelcontribcol binary by updating the components.go file.
    The component must be enabled only after sufficient testing, and there is enough confidence in the stability and quality of the component.

@anuraaga
Copy link
Contributor

Just curious, is logsstdinreceiver a better name? Or would it be pretty straightforward to extend this receiver to support trace and metrics that are received in JSON format? If the latter isn't possible, I think I would add logs to the name.

@gramidt
Copy link
Member

gramidt commented Dec 29, 2020

I too agree with @anuraaga on the naming of the receiver. :)

Definitely would be interesting to think through the use cases of accepting metrics and trace data via stdin. If so, what format would it support? OTLP? If OTLP, then why would stdin be used instead of just using the OTLP HTTP/gRPC endpoints?

@anuraaga
Copy link
Contributor

Yeah figured it would be OTLP-JSON.

I don't know if there is a huge use case, at least yet, but one that comes to mind is that gRPC can be bad in serverless, if functions get cleaned or frozen frequently. Log messages are reliable to write though.

But yeah if the receiver has logs in its name than no problem. I just had an expectation that stdinreceiver would support multiple formats.

By the way, I didn't look at the code but does it have to be stdin or can it be any file? If so maybe logsfilereceiver. But no worries either way :)

@gramidt
Copy link
Member

gramidt commented Dec 30, 2020

The serverless use case is rather interesting, @anuraaga. I'll give this some thought. Thank you for the insight!

@github-actions
Copy link
Contributor

github-actions bot commented Jan 6, 2021

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jan 6, 2021
@tigrannajaryan tigrannajaryan marked this pull request as draft January 11, 2021 16:06
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jan 19, 2021
@github-actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Jan 27, 2021
dyladan referenced this pull request in dynatrace-oss-contrib/opentelemetry-collector-contrib Jan 29, 2021
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants