Skip to content

Conversation

@jonatan-ivanov
Copy link
Member

@jonatan-ivanov jonatan-ivanov commented May 28, 2025

See #6206

spryslmatej and others added 2 commits May 28, 2025 16:19
Signed-off-by: spryslmatej <spryslmatej@gmail.com>
@jonatan-ivanov
Copy link
Member Author

@spryslmatej I removed a few ctors to make the changes smaller (we should not support all the possible combinations or all the parameters). Could you please check if this is usable for you?

Copy link
Member

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

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

I wonder if we couldn't use the MeterRegistry's step by default if a refresh interval isn't provided. We could only do that for PushMeterRegistry with PushRegistryConfig, but it might alleviate some need to pass a custom refresh interval.

@jonatan-ivanov
Copy link
Member Author

jonatan-ivanov commented May 29, 2025

I like this idea, I assume you were also thinking about getting the step in bindTo from the registry and not letting the user pass the config to a ctor of KafkaMetrics.

I tried to implement it but I realized that our registries do not return their config objects.
MeterRegistry does not have a reference to MeterRegistryConfig and registries don't pass their config "up" to it by calling a super ctor (there is none they could call).

I'm not sure we can "fix" this on the MeterRegistry level but we can add a getter to PushMeterRegistry. The problem with that is since registries don't know their config type, we either return PushRegistryConfig everywhere (e.g. in StepMeterRegistry and in every other implementations) or do something like this:

public <T extends PushRegistryConfig> T getConfig() {
    return (T) config;
}

Let's discuss this internally and see what we can do.

@jonatan-ivanov
Copy link
Member Author

I created #6342, in the meantime, we can go with the refresh interval parameter in this PR.

@jonatan-ivanov jonatan-ivanov merged commit 43d1f76 into micrometer-metrics:main Jun 3, 2025
9 checks passed
@jonatan-ivanov jonatan-ivanov deleted the kafka-metrics-custom-refresh-interval branch June 3, 2025 02:06
@jonatan-ivanov
Copy link
Member Author

@spryslmatej A new SNAPSHOT release should be created in the next few minutes, could you please check if it works for you?

@spryslmatej
Copy link
Contributor

@jonatan-ivanov Tested with version 1.16.0-SNAPSHOT, works as expected 👍

Thank you very much!

izeye added a commit to izeye/micrometer that referenced this pull request Jun 8, 2025
Signed-off-by: Johnny Lim <izeye@naver.com>
jonatan-ivanov pushed a commit that referenced this pull request Jun 9, 2025
Signed-off-by: Johnny Lim <izeye@naver.com>
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.

3 participants