Skip to content
This repository has been archived by the owner on Mar 28, 2023. It is now read-only.

feat: add latency & count metrics for content routing client #59

Merged
merged 2 commits into from
Oct 19, 2022

Conversation

guseggert
Copy link
Contributor

No description provided.

@guseggert guseggert force-pushed the feat/metrics branch 3 times, most recently from 9dfc67d to a7c1f56 Compare October 17, 2022 17:53
@guseggert guseggert linked an issue Oct 17, 2022 that may be closed by this pull request
@guseggert guseggert requested a review from ajnavarro October 17, 2022 19:41
} else if errors.Is(err, context.DeadlineExceeded) {
errStr = "DeadlineExceeded"
} else {
errStr = "Unknown"
Copy link
Member

Choose a reason for hiding this comment

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

Do you think that might be a problem to set the err.Error() here instead of Unknown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, if the error message has something dynamic in it like an address or some data from the request then it could cause high cardinality which is bad (availability risk for Prometheus, makes the metrics noisy and expensive to query).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is why I also added a log statement, if you see "Unknown" and want to dig in more, you can look at logs.

@guseggert guseggert merged commit 98c6d42 into main Oct 19, 2022
@ajnavarro ajnavarro deleted the feat/metrics branch October 19, 2022 14:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add metrics
2 participants