Skip to content

backport server idle metric to 2.1 #4716

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Jul 9, 2024
Merged

backport server idle metric to 2.1 #4716

merged 9 commits into from
Jul 9, 2024

Conversation

@DomGarguilo
Copy link
Member

@DomGarguilo DomGarguilo commented Jul 1, 2024 •
edited
Loading

Fixes #4641

The changes in this PR will

@DomGarguilo DomGarguilo self-assigned this Jul 1, 2024
dlmarion
dlmarion reviewed Jul 1, 2024
View reviewed changes
Copy link
Contributor

@dlmarion dlmarion left a comment

Choose a reason for hiding this comment

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

#4078 also had a change to MetricsIT.

@DomGarguilo
Copy link
Member Author

DomGarguilo commented Jul 1, 2024

So far I have backported the changes as they exist in elasticity and tried to get the test working. Right now I can see the metric for the compactor and scan server in the test but am not seeing it for the tserver yet (no metric at all) and am not sure why.

@DomGarguilo
Copy link
Member Author

DomGarguilo commented Jul 1, 2024

but am not seeing it for the tserver yet (no metric at all) and am not sure why.

Looked into this a bit and it seems like micrometer only reports the metric after it changes and since the tserver never became idle, the value for this metric was never emitted.

@DomGarguilo DomGarguilo linked an issue Jul 3, 2024 that may be closed by this pull request
Investigate if both METRICS_COMPACTOR_BUSY and METRICS_SERVER_IDLE are needed in elasticity branch #4641
Closed
@DomGarguilo DomGarguilo changed the title WIP - backport server idle metric to 2.1 backport server idle metric to 2.1 Jul 5, 2024
@DomGarguilo
Copy link
Member Author

DomGarguilo commented Jul 5, 2024

removing the WIP label as I think this is ready for review now. The only thing I need to fix is the CompactorTest which is failing in the builds. I am not sure whats going on there, maybe something wrong with the mocking in that test now.

dlmarion
dlmarion approved these changes Jul 8, 2024
View reviewed changes
Copy link
Contributor

@dlmarion dlmarion left a comment

Choose a reason for hiding this comment

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

Looks good. Just a minor comment regarding the property description.

@DomGarguilo DomGarguilo merged commit ffbbca7 into apache:2.1 Jul 9, 2024
@DomGarguilo DomGarguilo deleted the serverIdleMetrics branch July 9, 2024 19:41
@DomGarguilo DomGarguilo mentioned this pull request Jul 10, 2024
Closed
@ctubbsii ctubbsii added this to the 2.1.3 milestone Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dlmarion dlmarion dlmarion approved these changes

Assignees

@DomGarguilo DomGarguilo

Labels

None yet

Projects

None yet

Milestone

2.1.3

Development

Successfully merging this pull request may close these issues.

Investigate if both METRICS_COMPACTOR_BUSY and METRICS_SERVER_IDLE are needed in elasticity branch

3 participants

@DomGarguilo @dlmarion @ctubbsii