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

feat: add separate flag for metrics #1504

Merged
merged 12 commits into from
Oct 25, 2024
Merged

feat: add separate flag for metrics #1504

merged 12 commits into from
Oct 25, 2024

Conversation

AntiTyping
Copy link
Contributor

@AntiTyping AntiTyping commented Sep 16, 2024 •
edited
Loading

Refactors debug and metrics servers into two separate servers

  • adds --enable-metrics-server
  • uses 127.0.0.1 as the default address for debug and metrics servers
  • uses port 5184 as the default port for metrics server
  • adds metrics-listen-addr setting to config.yaml
  • rly config init will generate the following config
  • renamed api-listen-addr to debug-listen-addr
  • renames --debug-addr to --debug-listen-addr to be consistent with config
global:
    debug-listen-addr: 127.0.0.1:5183
    metrics-listen-addr: 127.0.0.1:5184
    ...
  • manually test the relayer to make sure it works today
  • add unit tests to make sure it works today and in the future
  • old --debug-addr should still work but with deprication message
  • re-test manually

@AntiTyping AntiTyping requested a review from a team as a code owner September 16, 2024 20:12
@AntiTyping AntiTyping force-pushed the andy/feat/add-separate-flag-for-metrics branch from 709729a to 17a9b80 Compare September 16, 2024 20:27
@freak12techno
Copy link
Contributor

Just saying, 9100 is a default port for node_exporter, which is widely used everywhere for HW metrics collection, so it might clash.

Reecepbcups
Reecepbcups reviewed Sep 17, 2024
View reviewed changes
cmd/flags.go Outdated Show resolved Hide resolved
@Reecepbcups Reecepbcups changed the title Andy/feat/add separate flag for metrics feat: add separate flag for metrics Sep 17, 2024
jtieri
jtieri reviewed Sep 17, 2024
View reviewed changes
cmd/flags.go Outdated Show resolved Hide resolved
cmd/config.go Outdated Show resolved Hide resolved
@AntiTyping AntiTyping force-pushed the andy/feat/add-separate-flag-for-metrics branch 3 times, most recently from 6edcf5c to ec7bb41 Compare September 19, 2024 15:58
jtieri
jtieri reviewed Sep 27, 2024
View reviewed changes
README.md Show resolved Hide resolved
cmd/config.go Show resolved Hide resolved
cmd/config_test.go Outdated Show resolved Hide resolved
cmd/flags.go Show resolved Hide resolved
cmd/flags.go Show resolved Hide resolved
cmd/start.go Show resolved Hide resolved
cmd/start_test.go Show resolved Hide resolved
cmd/start_test.go Show resolved Hide resolved
cmd/start_test.go Show resolved Hide resolved
examples/config_EXAMPLE.yaml Show resolved Hide resolved
@jtieri
Copy link
Member

jtieri commented Oct 17, 2024

Did we still want to ensure backwards compatibility in this PR by adding back the old flags/old fields in the config, and then mark things deprecated for one release?

If not then i think this is mostly ready to merge and we can just cut a new major release

@AntiTyping
Copy link
Contributor Author

@jtieri I restored the legacy flags and settings in this commit 6cb53dc

@jtieri jtieri enabled auto-merge (squash) October 21, 2024 17:51
@jtieri jtieri disabled auto-merge October 21, 2024 18:58
jtieri
jtieri approved these changes Oct 21, 2024
View reviewed changes
Copy link
Member

@jtieri jtieri left a comment

Choose a reason for hiding this comment

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

I just had the one comment about logs but other than that this looks good! I'll go ahead and approve now so merging is not blocked on me

cmd/start.go Outdated Show resolved Hide resolved
@AntiTyping AntiTyping force-pushed the andy/feat/add-separate-flag-for-metrics branch from f39c178 to ff05eaf Compare October 23, 2024 19:02
@AntiTyping
feat: adds metrics server address to config and sets localhost defaults
5f1259c 
* uses metrics-listen-addr for metrics server
* sets default debug server api-listen-addr to 127.0.0.1:5183
* sets default metrics server metrics-listen-addr to 127.0.0.1:9100
@AntiTyping
refactor: separates debug and metrics server
b3779b4 
* moves metrics to separate server
@AntiTyping
feat: changes the default metrics port to 5184
16a7bc8
@AntiTyping
refactor: removes address flags for debug and metrics server
155aa60 
* removes address flags so config file is the source of truth for addresses
* adds tests for flags to enable debug and metrics servers
* adds tests for config file settings for addresses for debug and metrics servers
* adjusts log messages
@AntiTyping
refactor: renames api-listen-addr to debug-listen-addr
ffa1ef3
@AntiTyping
feat: restores cli flags to set addresses for debug and metrics servers
fd2fc85 
* restores address flags
* renames `--debug-addr` to `--debug-listen-addr` to be consistent with config file setting `debug-listen-addr:`
* renames `--metrics-addr` to `--metrics-listen-addr` to be consistent with config file setting `metrics-listen-addr:`
* updates docs
@AntiTyping
feat: improves error reporting
57923f4
@AntiTyping
chore: code cleanup
b0e2525
@AntiTyping
feat: rollbacks --debug-addr flag
b73998f
@AntiTyping
feat: deprecates api-listen-addr
172fa92
@AntiTyping
fix: closes body
a1b3899
@AntiTyping
fix: reformat log messages to follow styleguide
56008aa
@AntiTyping AntiTyping force-pushed the andy/feat/add-separate-flag-for-metrics branch from ff05eaf to 56008aa Compare October 23, 2024 19:09
@AntiTyping AntiTyping merged commit 2cd9062 into cosmos:main Oct 25, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Reecepbcups Reecepbcups Reecepbcups left review comments

@jtieri jtieri jtieri approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AntiTyping @freak12techno @jtieri @Reecepbcups