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

Switch consumer to per-signal package #10220

Closed
wants to merge 12 commits into from

Conversation

dmathieu
Copy link
Member

@dmathieu dmathieu commented May 24, 2024

Description

This is a proposal showing the changes in the consumer package, as described in #10207.
With this change, the consumer package (and subpackages) can become stable, and new signals (such as profiles) can be added as new modules that aren't stable.

Testing

This is unit-tested.

@dmathieu dmathieu force-pushed the consumer-signals branch 15 times, most recently from 0438805 to eb1a2a0 Compare May 24, 2024 11:27
Copy link

codecov bot commented May 24, 2024

Codecov Report

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

Project coverage is 92.54%. Comparing base (5ba6000) to head (246c770).

Files Patch % Lines
consumer/internal/consumertest/consumer.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10220      +/-   ##
==========================================
+ Coverage   92.53%   92.54%   +0.01%     
==========================================
  Files         387      397      +10     
  Lines       18191    18252      +61     
==========================================
+ Hits        16833    16892      +59     
- Misses       1019     1020       +1     
- Partials      339      340       +1     

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

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Feedback about implementation (does not mean I agree with the split decision):

  1. clog - is a bad name since we may have to use something like this for component as well and may confuse between consumer vs component, maybe conslog?
  2. Do we also need to split consumertest package? Do we need to rename it to be consistent to something like ctest (or constest)?
  3. What about consumererror? Same question about naming.

@dmathieu
Copy link
Member Author

dmathieu commented May 27, 2024

I have renamed clog to conslog (did you want the same rename for ctrace and cmetric?)
I have also split consumertest and consumererror into per-signal packages, for their signal-specific things.

@dmathieu dmathieu closed this May 29, 2024
@dmathieu dmathieu deleted the consumer-signals branch May 29, 2024 16:14
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