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

Merge change from #220 (node aggregated metrics) to fs/branch_9x #229

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

patsonluk
Copy link
Collaborator

@patsonluk patsonluk commented Oct 10, 2024

Description

#220 merged it to fs/branch_9_3, So we are pulling that change to fs/branch_9x as well

@patsonluk patsonluk force-pushed the patsonluk/aggregate_prometheus_metrics_9x branch from 94d3167 to ca7dc01 Compare October 10, 2024 18:42
Copy link
Collaborator

@magibney magibney left a comment

Choose a reason for hiding this comment

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

I really think we should split the "autoCommit" metrics into a separate PR at least. But more generally: how badly do we need the autocommit metrics? If we think this info is useful, then we should upstream it ... it's certainly generic enough. EDIT: nevermind I see this was already here, fs-specific, just adjusting how it's done.

Also, just to be sure: the CoreMetricsAPICaller doesn't actually request metrics that were already found via the solr.node aggregate metrics group, right? Basically I'm hoping to confirm that we're saving effort by directly getting pre-aggregated metrics, and not doing both ways "just in case". I think that's how it's working, just looking to confirm.

Copy link
Collaborator

@magibney magibney left a comment

Choose a reason for hiding this comment

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

LGTM, assuming a positive confirmation of my earlier question:

Basically I'm hoping to confirm that we're saving effort by directly getting pre-aggregated metrics, and not doing both ways "just in case".

@patsonluk
Copy link
Collaborator Author

LGTM, assuming a positive confirmation of my earlier question:

Basically I'm hoping to confirm that we're saving effort by directly getting pre-aggregated metrics, and not doing both ways "just in case".

Yes AggregateMetricsApiCaller will go through all the values declared in CoreMetric enum, if it's not found in the node aggregated metrics, it will put that into the missingCoreMetrics list (there will be a fix in another PR for concurrent access issue), which such list is picked up by CoresMetricsApiCaller which builds the query string based on the missingCoreMetrics list. So the second call from CoresMetricsApiCaller should only query on whatever is not yet covered by AggregateMetricsApiCaller

@patsonluk patsonluk merged commit fcd9ebb into fs/branch_9x Oct 10, 2024
7 checks passed
magibney pushed a commit that referenced this pull request Oct 11, 2024
@magibney
Copy link
Collaborator

force-pushed to adjust the commit message on this merge -- the new merged commit is 0fd93bb

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.

2 participants