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

[#4859] Change default value of flag metric_node_name to hostname:port #5014

Merged
merged 4 commits into from
Jul 28, 2020

Conversation

vikramrajsitpal
Copy link
Contributor

@vikramrajsitpal vikramrajsitpal commented Jul 8, 2020

prometheus-metrics: change default metric_node_name to hostname:port

Default value for GFLAG metric_node_name was "DEFAULT_NODE_NAME". Changed this to hostname:web_server_port.
/prometheus-metrics now show new default value.

Summary:
Test (on localhost):
Command: ./bin/yb-ctl start --rf 1
On opening http://127.0.0.1:7000/prometheus-metrics, the exported_instance field shows hostname:webserverPort

Command with custom metric_node_name: ./bin/yb-ctl start --rf 1 --master_flags "metric_node_name=alpha:1"
On opening http://127.0.0.1:7000/prometheus-metrics, the exported_instance field shows alpha:1

@bmatican bmatican requested a review from iSignal July 9, 2020 15:52
@iSignal
Copy link
Contributor

iSignal commented Jul 9, 2020

Looks good in general @vikramrajsitpal , I will run this through our build and unit test pipeline and get back to you.

@iSignal
Copy link
Contributor

iSignal commented Jul 9, 2020

Can you please update the PR summary with a test plan? For ex: are change reflected in http://127.0.0.1:7000/prometheus-metrics when the flag is not specified vs explicitly specified?

@vikramrajsitpal
Copy link
Contributor Author

Can you please update the PR summary with a test plan? For ex: are change reflected in http://127.0.0.1:7000/prometheus-metrics when the flag is not specified vs explicitly specified?

Done.

Copy link
Contributor

@iSignal iSignal left a comment

Choose a reason for hiding this comment

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

Minor formatting comments. I will let you know what the results of the internal test pipeline are - just in case there are any unit tests that depend on this flag value.

src/yb/master/master_main.cc Outdated Show resolved Hide resolved
src/yb/master/master_main.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@iSignal iSignal left a comment

Choose a reason for hiding this comment

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

We should also need to set this flag by default for tablet servers too, so I would also add similar code to tablet_server_main.cc.

src/yb/master/master_main.cc Outdated Show resolved Hide resolved
src/yb/master/master_main.cc Outdated Show resolved Hide resolved
@vikramrajsitpal
Copy link
Contributor Author

Changes added, tests performed for master and tablet server.

Command: ./bin/yb-ctl start --rf 2 --master_flags "metric_node_name=malpha:1" --tserver_flags "metric_node_name=talpha:88"

On opening http://127.0.0.1:9000/prometheus-metrics, the exported_instance field shows talpha:88.
Without the custom flag metric_node_name in --tserver_flags, the default value of hostname:9000 is shown.

@iSignal iSignal changed the title Issue #4859 [#4859] Change default value of flag metric_node_name to hostname:port Jul 20, 2020
Copy link
Contributor

@iSignal iSignal left a comment

Choose a reason for hiding this comment

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

Looks good. I will run it through the internal tests once again to make sure nothing changes.

A couple of minor comments.

src/yb/master/master_main.cc Outdated Show resolved Hide resolved
src/yb/tserver/tablet_server_main.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@iSignal iSignal left a comment

Choose a reason for hiding this comment

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

Thanks for your patience @vikramrajsitpal ! Just one minor indentation issue remaining.

src/yb/master/master_main.cc Outdated Show resolved Hide resolved
src/yb/tserver/tablet_server_main.cc Outdated Show resolved Hide resolved
@vikramrajsitpal
Copy link
Contributor Author

Had to force push as the commit message had a typo in the Issue number.

@iSignal
Copy link
Contributor

iSignal commented Jul 28, 2020

There is a conflict in the CONTRIBUTORS.md file, apparently, could you rebase your branch to the latest commit in master, fix the conflict and possibly force push again?

@vikramrajsitpal
Copy link
Contributor Author

There is a conflict in the CONTRIBUTORS.md file, apparently, could you rebase your branch to the latest commit in master, fix the conflict and possibly force push again?

So, I first merged my forked repo's master branch with yugabyte-db/master. Then I rebased my fix branch which is used in this PR and resolved the conflict. I have pushed my fix branch for now and it is visible here, but, it has an extra commit which it prompted me to make when I finished merging my master branch with ydb's master branch, so that is also visible. Please let me know if it is not acceptable.

…ame to hostname:port

Default value for GFLAG metric_node_name was "DEFAULT_NODE_NAME". Changed this to hostname:web_server_port.
<target>/prometheus-metrics now show new default value.
… of metric_node_name for tablet servers

As per PR review comments, made minor changes to styling and GetHostName(). No change in logic.
Also changed the default value for FLAG metric_node_name to hostname:webserverPort for the tablet server.
Used strings::Substitute as per the review comment.
@iSignal iSignal merged commit 646c802 into yugabyte:master Jul 28, 2020
@vikramrajsitpal vikramrajsitpal deleted the fix-4859 branch July 29, 2020 00:44
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.

None yet

2 participants