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

Support configuration of cardinality limit in autoconfigure #5662

Closed

Conversation

akats7
Copy link

@akats7 akats7 commented Jul 28, 2023

This PR adds support for configuring the cardinality limit when using the autoconfigure package. The configured cardinality limit will be applied to the default registeredViews in the viewRegistry.

Test cases have been added to test if the cardinality has been properly applied to all default views.

Resolves #5639

@akats7 akats7 requested a review from a team July 28, 2023 03:36
@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (ad6cbfc) 91.31% compared to head (e8d10b7) 91.31%.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #5662   +/-   ##
=========================================
  Coverage     91.31%   91.31%           
- Complexity     4978     4980    +2     
=========================================
  Files           554      554           
  Lines         14731    14735    +4     
  Branches       1376     1376           
=========================================
+ Hits          13451    13455    +4     
  Misses          884      884           
  Partials        396      396           
Files Changed Coverage Δ
.../sdk/autoconfigure/MeterProviderConfiguration.java 100.00% <100.00%> (ø)

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

@jack-berg
Copy link
Member

Ah.. I should have coordinated with you 😞 #5659

@akats7
Copy link
Author

akats7 commented Jul 28, 2023

Ah.. I should have coordinated with you 😞 #5659

Gotcha, I mentioned that I was planning on making the contribution in the description but should've assigned the issue to myself for clarity

@breedx-splk
Copy link
Contributor

Dang. 🙃 These are remarkably similar implementations as well.

@akats7 If it's ok with you (no offense at all!), I would slightly prefer to go with Jack's PR because:

  • Jack's includes update to the autoconfigure docs
  • Your assert method is marked @Test which is unusual since it's also called by other test methods
  • Jack's validates that the value is >= 1.

@akats7
Copy link
Author

akats7 commented Jul 28, 2023

No worries, important thing is that the functionality is added. And yep the Test annotation is a typo lol

@jack-berg
Copy link
Member

#5659 has been merged so I'm going to close this. Thanks anyway!

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.

Add cardinality configuration option to sdk autoconfigure
3 participants