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.

Sign up for GitHub

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

Jump to bottom

Metrics with service names #2908

Merged
merged 2 commits into from
Jan 6, 2023
Merged

Metrics with service names #2908

merged 2 commits into from
Jan 6, 2023

Conversation

varsha888
Copy link
Contributor

@varsha888 varsha888 commented Nov 29, 2022 •
edited
Loading

Motivation

Prepend service names to metrics for better tracking info
#2577

Screen Shot 2022-11-30 at 1 51 54 PM

@varsha888 varsha888 marked this pull request as draft November 29, 2022 18:41
jgreat
jgreat reviewed Nov 29, 2022
View reviewed changes
fog/ledger/server/src/lib.rs Outdated Show resolved Hide resolved
fog/ingest/server/src/lib.rs Outdated Show resolved Hide resolved
fog/report/server/src/lib.rs Outdated Show resolved Hide resolved
util/grpc/src/lib.rs Outdated Show resolved Hide resolved
@varsha888 varsha888 marked this pull request as ready for review November 30, 2022 21:53
@jcape jcape requested a review from a team December 1, 2022 18:20
awygle
awygle approved these changes Dec 2, 2022
View reviewed changes
Copy link
Contributor

@awygle awygle left a comment

Choose a reason for hiding this comment

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

This looks good implementation-wise, I just had a couple of minor suggestions.

util/metrics/src/service_metrics.rs Outdated Show resolved Hide resolved
util/grpc/src/lib.rs Outdated Show resolved Hide resolved
@varsha888 varsha888 force-pushed the metrics branch 2 times, most recently from 7c8d7b9 to c2483af Compare December 2, 2022 20:06
@jcape
Copy link
Contributor

jcape commented Dec 6, 2022

Unfortunately this isn't passing CI, and it looks like it's hanging in consensus-tests 2, 2 because Cancelled after 360m (i.e. all the tests hang for 6 hours), my suspicion here is that something about that test is preventing us from having two SVC_COUNTERS variables active at a given time (i.e. the test assumes there's only one).

@varsha888 varsha888 force-pushed the metrics branch 3 times, most recently from 95f69de to 75bd296 Compare December 8, 2022 07:02
@varsha888 varsha888 merged commit 2b82291 into master Jan 6, 2023
@varsha888 varsha888 deleted the metrics branch January 6, 2023 19:56
briancorbin pushed a commit that referenced this pull request Jan 23, 2023
@varsha888
Metrics with service names (#2908)
09972bc 
* Metrics with service names
* bump postgresql version
briancorbin pushed a commit to ryankurte/mobilecoin that referenced this pull request Jan 23, 2023
@varsha888
Metrics with service names (mobilecoinfoundation#2908)
cf2909e 
* Metrics with service names
* bump postgresql version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iamalwaysuncomfortable iamalwaysuncomfortable iamalwaysuncomfortable left review comments

@jgreat jgreat jgreat approved these changes

@awygle awygle awygle approved these changes

@jcape jcape Awaiting requested review from jcape

Assignees

@varsha888 varsha888

Labels
None yet
Projects
Blockchain Core
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@varsha888 @jcape @jgreat @awygle @iamalwaysuncomfortable