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 event addition metrics for latency and counter #4836

Closed
stevend-uber opened this issue Jan 26, 2024 · 3 comments
Closed

Add event addition metrics for latency and counter #4836

stevend-uber opened this issue Jan 26, 2024 · 3 comments
Labels
help wanted Issues with this label are ready to start work but are in need of someone to do it priority/backlog Issue is approved and in the backlog

Comments

@stevend-uber
Copy link
Contributor

Currently in the event-based cache hydration feature, we have metrics around latency/counts for fetch and prunes of events from the events table. I propose we add a counter and latency metrics around Event-Addition.

@rturner3 rturner3 added help wanted Issues with this label are ready to start work but are in need of someone to do it priority/backlog Issue is approved and in the backlog labels Feb 1, 2024
@Victorblsilveira
Copy link

Most of the metrics creation is done at metricsWrapper. Another aspect of this is that event creation is done in createRegistrationEntryEvent, an internal function. Even if we add the function to the interface and implement the metrics start at metricsWrapper, most internal calls wouldn't fire the latency/count metric.

One solution would be to internalize the metrics fire to the sqlstore.go. Although looking at the code this breaks the patterns. Is that an acceptable solution for the spire community? I want to help here and appreciate any guidance.

@rturner3
Copy link
Collaborator

@Victorblsilveira Your suggested approach seems reasonable to me.

The only alternative I could think of would be to refactor the datastore interface to make the creation of events an explicit call from the entry cache manager. But that's not really ideal because it introduces the possibility of business logic bugs where events are mistakenly not written to the database table for write/delete operations to entries/attested nodes.

@azdagron
Copy link
Member

azdagron commented Jun 4, 2024

Talking in the contributor sync, we determined that with #4720 we should have enough metrics to start with for the events based cache.

@azdagron azdagron closed this as completed Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issues with this label are ready to start work but are in need of someone to do it priority/backlog Issue is approved and in the backlog
Projects
None yet
Development

No branches or pull requests

4 participants