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

A request to expose the latest revision and latest committed at of the watcher as metrics #1092

Open
be-hase opened this issue Feb 5, 2025 · 6 comments · May be fixed by #1094
Open

A request to expose the latest revision and latest committed at of the watcher as metrics #1092

be-hase opened this issue Feb 5, 2025 · 6 comments · May be fixed by #1094
Milestone

Comments

@be-hase
Copy link
Member

be-hase commented Feb 5, 2025

Hello, I have a feature request for centraldogma.
How about adding the following metrics?

centraldogma_latest_commited_at{project="xxx", repository="xxx", watcher="xxx"} timestamp
centraldogma_latest_revision{project="xxx", repository="xxx", watcher="xxx"} revisionNumber

Motivation:
Having these metrics would allow us to represent changes in centraldogma using Grafana annotations.
Many services use centraldogma as a feature flag system, and being able to visualize changes in Grafana would be highly useful.

@ikhoon ikhoon added this to the 0.74.0 milestone Feb 5, 2025
@be-hase
Copy link
Member Author

be-hase commented Feb 5, 2025

Approach

I am considering the following approach:

First, I will add a module named centraldogma-client-micrometer.
Within this module, I will add a class called WatcherMetrics, which will be executed as follows:

Watcher watcher = ...;
WatcherMetrics.monitor(watcher);

A similar implementation can be found in the following examples:

Why separate the module?

centraldogma-client does not have a dependency on Micrometer.

Question

Is it possible to retrieve the committed time?
It seems that the Revision class does not include it.

@ikhoon
Copy link
Contributor

ikhoon commented Feb 5, 2025

centraldogma-client does not have a dependency on Micrometer.

I prefer adding Micrometer dependency to centraldogma-client module because Micrometer is commonly used in Java ecosystem so it may not be an additional dependency in production.

Within this module, I will add a class called WatcherMetrics, which will be executed as follows:

The watch metrics seem useful for most users. What do you think of automatically adding the watcher metrics if a MeterRegistry is registered to ArmeriaCentralDogmaBuilder?

new ArmeriaCentralDogmaBuilder()
   .meterReigistry(meterRegistry, meterIdPrefix)
   // If the prefix is omitted, it defaults to `centraldogma.client`.
   .meterReigistry(meterRegistry)
   .build();

// The watcher exposes metrics if meterRegistry is set until closed.
Watcher watcher = ...;

@ikhoon
Copy link
Contributor

ikhoon commented Feb 5, 2025

Additionally, it would be perfect if we could integrate MeterRegistry for Spring integration modules.

public CentralDogma dogmaClient(
Environment env,
CentralDogmaSettings settings,
Optional<List<CentralDogmaClientFactoryConfigurator>> factoryConfigurators,
Optional<ArmeriaClientConfigurator> armeriaClientConfigurator,
Optional<DnsAddressEndpointGroupConfigurator> dnsAddressEndpointGroupConfigurator)

If a MeterRegistry is registered as a bean, the injected bean could be used to create ArmeriaCentralDogma internally.

@be-hase
Copy link
Member Author

be-hase commented Feb 5, 2025

Micrometer is commonly used in Java ecosystem so it may not be an additional dependency in production.

I agree.
In fact, centraldogma-client-armeria already has a dependency on Micrometer through Armeria...

What do you think of automatically adding the watcher metrics if a MeterRegistry is registered to ArmeriaCentralDogmaBuilder?

Let's go with that approach. If we want to add more metrics to the Central Dogma client in the future, this structure seems more convenient.

@ikhoon
Copy link
Contributor

ikhoon commented Feb 5, 2025

If we want to add more metrics to the Central Dogma client in the future

Yes, we are.

@be-hase be-hase linked a pull request Feb 6, 2025 that will close this issue
2 tasks
@be-hase
Copy link
Member Author

be-hase commented Feb 6, 2025

I am considering this approach. What do you think?
#1094

@minwoox minwoox modified the milestones: 0.74.0, 0.75.0 Mar 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants