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

[receiver/nopreceiver] Add the nopreceiver #9446

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

evan-bradley
Copy link
Contributor

Description:

Add the nopreceiver. This can be helpful if a user wants to start the Collector with a dummy pipeline to only enable extensions. It could also be used to start a dynamically-configured Collector that starts with no config and waits to receive its config from a confmap.Provider that supports reloads.

Link to tracking Issue:

Works toward #7316

Testing:

Added lifecycle tests; the receiver doesn't do anything.

Documentation:

Added a readme for the component.

@evan-bradley evan-bradley requested a review from a team as a code owner January 31, 2024 19:34
@evan-bradley
Copy link
Contributor Author

cc @djaglowski @tigrannajaryan

Copy link

codecov bot commented Jan 31, 2024

Codecov Report

Attention: Patch coverage is 73.33333% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 90.93%. Comparing base (2413346) to head (e4c3692).

Files Patch % Lines
.../nopreceiver/internal/metadata/generated_status.go 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9446      +/-   ##
==========================================
- Coverage   90.95%   90.93%   -0.02%     
==========================================
  Files         350      352       +2     
  Lines       18576    18591      +15     
==========================================
+ Hits        16895    16906      +11     
- Misses       1354     1358       +4     
  Partials      327      327              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

LGTM, minor CI issues aside

@evan-bradley
Copy link
Contributor Author

evan-bradley commented Jan 31, 2024

Codecov was complaining that I wasn't testing the generated Meter or Tracer functions. I already had to manually edit the file to satisfy make goimpi, so I just removed them. If we want to take another approach to this I'm open to suggestions. I could test these or even try to use them in the code if we wanted.

Edit: this fails another check, I had to change my approach. See below.

@evan-bradley
Copy link
Contributor Author

It looks like I need to leave in the Meter and Tracer functions. Is the codecov check required to pass to merge this? None of the other components appear to test these and I'd prefer not to write tests just to increase code coverage.

@evan-bradley evan-bradley force-pushed the nopreceiver branch 7 times, most recently from 41a89fb to bbacb14 Compare February 6, 2024 18:32
Copy link
Member

@andrzej-stencel andrzej-stencel left a comment

Choose a reason for hiding this comment

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

Love it! ❤️

@evan-bradley evan-bradley force-pushed the nopreceiver branch 2 times, most recently from f815130 to d90934f Compare February 13, 2024 18:26
@codeboten
Copy link
Contributor

@evan-bradley ptal at the check failure

@evan-bradley evan-bradley force-pushed the nopreceiver branch 2 times, most recently from 1c9151c to f3af92c Compare February 16, 2024 13:02
@evan-bradley
Copy link
Contributor Author

@codeboten should be good now, thanks. The codecov check is still failing due to a lack of test coverage on generated code, but it looks like that's an optional check.

Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

LGTM. Please rebase and update the dependancies

@evan-bradley
Copy link
Contributor Author

@dmitryax Thanks for taking a look. Should be good to go now.

@evan-bradley evan-bradley force-pushed the nopreceiver branch 2 times, most recently from ee0a1af to 7e7fa11 Compare March 7, 2024 03:48
@evan-bradley evan-bradley force-pushed the nopreceiver branch 2 times, most recently from e5b9742 to b99f69d Compare March 8, 2024 20:37
@evan-bradley
Copy link
Contributor Author

@dmitryax CI is green now, minus the code coverage failure due to the lack of tests for generated code.

@dmitryax dmitryax merged commit 47de864 into open-telemetry:main Mar 12, 2024
46 of 47 checks passed
@github-actions github-actions bot added this to the next release milestone Mar 12, 2024
tomasmota pushed a commit to tomasmota/opentelemetry-collector that referenced this pull request Mar 14, 2024
**Description:**

Add the nopreceiver. This can be helpful if a user wants to start the
Collector with a dummy pipeline to only enable extensions. It could also
be used to start a dynamically-configured Collector that starts with no
config and waits to receive its config from a confmap.Provider that
supports reloads.

**Link to tracking Issue:**

Works toward
open-telemetry#7316

**Testing:**

Added lifecycle tests; the receiver doesn't do anything.

**Documentation:**

Added a readme for the component.
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.

None yet

6 participants