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

feat(core): Enable metrics for core package #2863

Merged
merged 9 commits into from
Nov 20, 2023

Conversation

renaynay
Copy link
Member

@renaynay renaynay commented Oct 19, 2023

While we do plan to remove the bridge node type eventually, it's useful to have some visibility into the core package in the short term.

@renaynay renaynay changed the title Metrics bridge feat(core): Enable metrics for core package Oct 24, 2023
@renaynay renaynay added area:core_and_app Relationship with Core node and Celestia-App kind:feat Attached to feature PRs area:metrics Related to measuring/collecting node metrics labels Oct 24, 2023
@renaynay renaynay self-assigned this Oct 24, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 26, 2023

Codecov Report

Attention: 86 lines in your changes are missing coverage. Please review.

Comparison is base (3fde8c9) 51.67% compared to head (2737752) 51.38%.

Files Patch % Lines
core/listener_metrics.go 8.51% 43 Missing ⚠️
core/exchange_metrics.go 18.51% 21 Missing and 1 partial ⚠️
core/listener.go 52.63% 7 Missing and 2 partials ⚠️
core/exchange.go 60.00% 6 Missing and 2 partials ⚠️
core/option.go 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2863      +/-   ##
==========================================
- Coverage   51.67%   51.38%   -0.29%     
==========================================
  Files         167      170       +3     
  Lines       10812    10925     +113     
==========================================
+ Hits         5587     5614      +27     
- Misses       4729     4810      +81     
- Partials      496      501       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@renaynay renaynay marked this pull request as ready for review October 26, 2023 08:45
core/metrics.go Outdated Show resolved Hide resolved
core/metrics.go Outdated Show resolved Hide resolved
core/metrics.go Outdated Show resolved Hide resolved
core/metrics.go Outdated Show resolved Hide resolved
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

I realized that we don't need blockTime metric. The bridge node publishes headers through Subscriber, which goes through that validator, and time is tracked there on subscriber metrics. So effectively we will get the same time twice

@renaynay
Copy link
Member Author

@Wondertan the reason i wanted to capture block time here is that it's the only way to know on the DA-side what the approximate block time is so that we can see if there's a difference between core network block time and DA header propagation time.

I removed it in this commit 403bc9a

@Wondertan
Copy link
Member

I understand and it turns out we designed things in a way that we don't need to duplicate anything 😺

@Wondertan
Copy link
Member

We should aim to include fetcher metrics here as well

@Wondertan
Copy link
Member

Linter btw

@renaynay renaynay marked this pull request as draft October 30, 2023 13:38
@renaynay renaynay marked this pull request as ready for review November 6, 2023 11:15
@renaynay renaynay marked this pull request as draft November 6, 2023 11:15
@renaynay renaynay marked this pull request as ready for review November 7, 2023 15:37
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

LGTM

@renaynay renaynay enabled auto-merge (squash) November 20, 2023 09:57
@renaynay renaynay merged commit a4ef6d1 into celestiaorg:main Nov 20, 2023
15 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core_and_app Relationship with Core node and Celestia-App area:metrics Related to measuring/collecting node metrics kind:feat Attached to feature PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants