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

Consolidate Metrics Query and Metrics Proccessor Services #15870

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

bhardwaj-priyanshu
Copy link
Contributor

@bhardwaj-priyanshu bhardwaj-priyanshu commented Feb 10, 2025

Metrics Query and Metrics Proccessor runs on the same pod, but consumes 2 cluster IP. This PR consolidates both the services into Metrics saving one cluster IP.

Testing

  • Tested by replacing the image and verified the metrics emmited.

@bhardwaj-priyanshu bhardwaj-priyanshu added the build Triggers github actions build label Feb 10, 2025
@bhardwaj-priyanshu bhardwaj-priyanshu force-pushed the feature/combine-metric-ip branch 7 times, most recently from 2acedb5 to d49ec7f Compare February 12, 2025 19:41
@bhardwaj-priyanshu bhardwaj-priyanshu marked this pull request as ready for review February 12, 2025 19:58
@@ -53,7 +53,7 @@ private enum AllowedMethod {
Constants.Service.LOG_QUERY);
public static final RouteDestination LOG_SAVER = new RouteDestination(Constants.Service.LOGSAVER);
public static final RouteDestination METRICS_PROCESSOR = new RouteDestination(
Constants.Service.METRICS_PROCESSOR);
Constants.Service.METRICS);
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace METRICS_PROCESSOR with METRICS in the below code.

@@ -525,7 +525,6 @@ private static List<Module> createPersistentModules(CConfiguration cConf, Config
cConf.set(Constants.Transaction.Container.ADDRESS, localhost);
cConf.set(Constants.Dataset.Executor.ADDRESS, localhost);
cConf.set(Constants.Metrics.ADDRESS, localhost);
cConf.set(Constants.MetricsProcessor.BIND_ADDRESS, localhost);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we bind Metrics service address here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its already binded

@bhardwaj-priyanshu bhardwaj-priyanshu force-pushed the feature/combine-metric-ip branch 2 times, most recently from 7ba682d to e5e4bdc Compare February 13, 2025 13:37
@bhardwaj-priyanshu bhardwaj-priyanshu marked this pull request as draft February 13, 2025 14:13
@bhardwaj-priyanshu bhardwaj-priyanshu removed the request for review from tivv February 13, 2025 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Triggers github actions build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants