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

Add support for snapshot metrics #295

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

patelsmit32123
Copy link

@patelsmit32123 patelsmit32123 commented Aug 9, 2024

Description

#165 Following the issue opened by ginkel, this PR adds the required changes for basic support for snapshot related metrics, starting with snapshot_age. More snapshot related metrics can be added on top of this changes.


  • All my commits include DCO.
    DCO stands for Developer Certificate of Origin and it is your declaration that your contribution is correctly attributed and licensed. Please read more about how to attach DCO to your commits here (spoiler alert: in most cases it is as simple as using -s option when doing git commit).
    Please be aware that commits without DCO will cause failure of PR CI workflow and can not be merged.

@lukas-vlcek
Copy link
Collaborator

lukas-vlcek commented Aug 9, 2024

FYI, to get rid of the Developer Certificate of Origin (DCO) check failure you need to sign all commits with -s option (git commit -s ...).

This will add a specific line into the commit message containing your signature with your email. See any other commit message in this repository for example of the text. You can create the signature manually in the commit message as well but using -s is more systematic way.

(Use of git -s option assume you configured git like this git config user.email "[email protected]" first.)

@patelsmit32123
Copy link
Author

@lukas-vlcek signed off the commit, please review

@patelsmit32123 patelsmit32123 force-pushed the main branch 3 times, most recently from 745948e to e3cd706 Compare August 10, 2024 18:50
@patelsmit32123
Copy link
Author

@lukas-vlcek I have added integration test and updated the README also, PTAL

@patelsmit32123 patelsmit32123 force-pushed the main branch 2 times, most recently from 72092d2 to a000692 Compare August 15, 2024 18:20
@patelsmit32123
Copy link
Author

@lukas-vlcek can you please help review this PR?

@lukas-vlcek
Copy link
Collaborator

@patelsmit32123 Sorry, I was on 🌴 last week. I will do it this week.

@lukas-vlcek
Copy link
Collaborator

@patelsmit32123 Thanks a lot for your patience! I am really sorry for the delay in processing this.

  • First of all, I am almost done with reviewing it and I already made some comments on your PR before, can you see them? (The review is not submitted yet so I do not know if GitHub shows my comments to you in this case)
  • Second, a new release 2.17.0.0 has been cut today, would you mind rebasing the PR?

@patelsmit32123
Copy link
Author

@lukas-vlcek i am not able to see your comments

Copy link
Collaborator

@lukas-vlcek lukas-vlcek left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. I am proposing some changes and enhancements. Let me know if anything is not clear.
Overall, great contribution, thanks for the effort.

build.gradle Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@lukas-vlcek
Copy link
Collaborator

@patelsmit32123 Can you see my comments now?

Signed-off-by: Smit Patel <[email protected]>
@patelsmit32123
Copy link
Author

@lukas-vlcek thanks for the review, resolved all comments

@patelsmit32123 patelsmit32123 changed the title Add support for snapshot related metrics Add support for snapshot metrics Oct 9, 2024
@patelsmit32123
Copy link
Author

@lukas-vlcek is there anything else needed to be done for this PR?

@lukas-vlcek
Copy link
Collaborator

@patelsmit32123 I think it is ready. I am trying to implement #315 now (your PR will benefit from it) but if a new release is needed before BWC tests are ready I will just go ahead and merge it to make it part of the next release.
Does it sound good?

Copy link
Collaborator

@lukas-vlcek lukas-vlcek left a comment

Choose a reason for hiding this comment

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

LGTM

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.

3 participants