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 gauge metrics to track missedEvents and cache sizes #5411

Merged
merged 2 commits into from
Aug 27, 2024

Conversation

stevend-uber
Copy link
Contributor

This PR adds metrics for the event-based cache. This will emit a gauge whenever the cache is updated for missed-events and current cache sizes.

Pull Request check list

  • Commit conforms to CONTRIBUTING.md?
  • Proper tests/regressions included?
  • Documentation updated?

Which issue this PR fixes
#4720

Copy link
Contributor

@edwbuck edwbuck left a comment

Choose a reason for hiding this comment

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

This heavily uses the word "missed" which in my mind implies that we forgot something; because, one only misses items that exist (or existed).

I would prefer using the word "skipped" as there are scenarios where we should skip event ids, because the events associated with them never were created (due to hung transactions that never completed, or autoincrement values that don't increase by one).

Also, we need updates to doc/telemetry/telemetry.md.

@stevend-uber stevend-uber force-pushed the stevend/gauge branch 6 times, most recently from 35ccd20 to 53f5776 Compare August 20, 2024 20:59
Copy link
Contributor

@edwbuck edwbuck left a comment

Choose a reason for hiding this comment

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

Thank you very much for these changes. They are a big contribution in monitoring a small value with big implications around the new database event cache.

Copy link
Contributor

@zmt zmt left a comment

Choose a reason for hiding this comment

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

LGTM. I left a few minor nits and some general pre-existing style questions.

@amartinezfayo amartinezfayo self-assigned this Aug 22, 2024
@stevend-uber stevend-uber force-pushed the stevend/gauge branch 2 times, most recently from a219c7e to 68047c5 Compare August 26, 2024 23:07
Signed-off-by: stevend <stevend@uber.com>
@amartinezfayo amartinezfayo added this to the 1.10.2 milestone Aug 27, 2024
Copy link
Member

@amartinezfayo amartinezfayo left a comment

Choose a reason for hiding this comment

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

Thank you @stevend-uber!

@amartinezfayo amartinezfayo merged commit e91897b into spiffe:main Aug 27, 2024
34 checks passed
amartinezfayo added a commit that referenced this pull request Aug 27, 2024
Signed-off-by: stevend <stevend@uber.com>
Co-authored-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
@azdagron azdagron mentioned this pull request Aug 28, 2024
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.

5 participants