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 the evict reason as a metric tag so we can categorize the evict reason. #195

Merged

Conversation

JoeShook
Copy link
Contributor

@JoeShook JoeShook commented Feb 7, 2024

I mentioned adding the reason tag in the OpenTelemetry support discussion. It may have gotten lost in the naming discussion that took over my comment.

Seems like a reasonable addition. What do you think? I used it in my plugin and in many working code deployments.

Thank you.

  • Joe

@jodydonetti jodydonetti self-requested a review February 7, 2024 22:56
@jodydonetti jodydonetti self-assigned this Feb 7, 2024
…fusioncache.memory.evict" used for the counter CounterMemoryEvict
@jodydonetti
Copy link
Collaborator

Hi Joe, nice idea!
We have to be careful when adding tags to metrics, since the ones with a high cardinality will slow down APM systems and/or make the bill higher (that's something I recently learned).

In this case though, the possible values are very limited so it LGTM, I just changed slightly the string identifier to be more aligned with an existing one.

What do you think?

@JoeShook
Copy link
Contributor Author

JoeShook commented Feb 8, 2024

That looks great!

Copy link
Collaborator

@jodydonetti jodydonetti left a comment

Choose a reason for hiding this comment

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

LGTM

@jodydonetti jodydonetti merged commit 80484d0 into ZiggyCreatures:main Feb 8, 2024
2 of 3 checks passed
@jodydonetti jodydonetti added this to the v1.0.0 milestone Feb 9, 2024
@jodydonetti
Copy link
Collaborator

Hi all, I just released v1.0.0-preview1 🥳

Please try it out so we can spot any potential issue before the official v1.0, thanks!

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