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 copy() method to Builders #4104

Closed
wants to merge 2 commits into from

Conversation

qweek
Copy link
Contributor

@qweek qweek commented Oct 3, 2023

Summary
This PR allows to reuse builders as templates for dynamic tags, factories, etc.

Changes
Introduce additional copy() method in all specific Meter's Builders, Tags and DistributionStatisticConfig.

Why is this important?
Currently, Micrometer doesn't provide a straightforward way to reuse builders. This PR eliminates the need to fully describe all fields for each similar builder, thereby simplifying metric management.

Backwards Compatibility
This feature is partially backward-compatible:

Test Coverage
CopyableTest class has simple test that verify new functionality, will extend it to cover all cases if you agree with design.

Usage Example
After this change, you can use new copy() method to create metric with dynamic tags

final Counter.Builder template = Counter.builder("counter");
template.copy().tags("tagA", "valueA").register(registry);
template.copy().tags("tagB", "valueB").register(registry);

@qweek
Copy link
Contributor Author

qweek commented Oct 3, 2023

Also suggest to make small breaking change and convert public DistributionStatisticConfig and DistributionStatisticConfig.Builder constructors to protected.
Now you can create new DistributionStatisticConfig(), but you can't change private fields.
You can create new DistributionStatisticConfig.Builder(), but also have builder() method with same functionality.

@jonatan-ivanov
Copy link
Member

Let me collect similar/involved issues/PRs here:

I think we should gather the use cases for this and what we can provide for this today, what changes might needed.

@qweek
Copy link
Contributor Author

qweek commented Oct 4, 2023

Some thoughts:

I prefer to split this problem into 2 parts:

  • make more flexible builders (make it reusable, add additional methods to gauge, maybe add name() method, validate builders)
  • collect use cases and find better solution for factories (delayed construction, caches, etc)

Right now, I know 2 use cases related to tags (dynamic tags and gauge with different objects/functions per dynamic tag)

@jonatan-ivanov
Copy link
Member

@qweek Would #4097 solve your first use-case with dynamic tags and would using MultiGauge help in the second?

@jonatan-ivanov jonatan-ivanov added the waiting for feedback We need additional information before we can continue label Jan 2, 2024
Copy link

If you would like us to look at this PR, please provide the requested information. If the information is not provided within the next 7 days this PR will be closed.

Copy link

Closing due to lack of requested feedback. If you would like us to look at this, please provide the requested information and we will re-open.

@github-actions github-actions bot closed this Jan 18, 2024
@jonatan-ivanov jonatan-ivanov added closed-as-inactive and removed waiting for feedback We need additional information before we can continue feedback-reminder labels Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants