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

Make metrics ip and port, and http endpoint port configurable #87

Merged
merged 9 commits into from
Sep 20, 2023

Conversation

mickel8
Copy link
Contributor

@mickel8 mickel8 commented Sep 18, 2023

This PR allows for running multiple JF instances in development. In particular:

  • allow for configuring prometheus metrics port and ip
  • allow for configuring http endpoint port in development
  • change ConfigParser to ConfigReader and move it to a separate file - runtime.exs is read after compilation so we can keep ConfigReader in lib directory making runtime.exs clearer
  • limit prod mandatory fields by generating SECRET_KEY_BASE when it's not passed and allowing for passing CHECK_ORIGIN not only in prod - this way we have only two prod mandatory variables i.e. VIRTUAL_HOST and SERVER_API_TOKEN.

@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

Merging #87 (cd00d5b) into main (bbff33a) will increase coverage by 0.56%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #87      +/-   ##
==========================================
+ Coverage   83.70%   84.27%   +0.56%     
==========================================
  Files          43       44       +1     
  Lines         718      744      +26     
==========================================
+ Hits          601      627      +26     
  Misses        117      117              
Files Changed Coverage Δ
lib/jellyfish/config_reader.ex 100.00% <100.00%> (ø)
lib/jellyfish_web/telemetry.ex 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbff33a...cd00d5b. Read the comment docs.

@mickel8 mickel8 marked this pull request as ready for review September 18, 2023 09:40
@mickel8 mickel8 requested review from roznawsk and Rados13 September 18, 2023 09:40
@mickel8 mickel8 changed the title Make metrics and endpoint ip and port configurable Make metrics ip and port, and http endpoint port configurable Sep 18, 2023
Comment on lines 124 to 130
# we set ip and port here to allow for
# running multiple Jellyfishes in development
config :jellyfish, JellyfishWeb.Endpoint,
# Binding to loopback ipv4 address prevents access from other machines.
# Change to `ip: {0, 0, 0, 0}` to allow access from other machines.
http: [ip: {127, 0, 0, 1}, port: port]

Copy link
Contributor

Choose a reason for hiding this comment

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

Won't it be confusing to set up JellyfishWeb.Endpoint in two places for the dev environment, now we set up JellyfishWeb.Endpoint in two places in this file, maybe we should move it closer and in the case clause?

@mickel8 mickel8 force-pushed the metrics-http-conf branch 2 times, most recently from 06b800e to 3b2008b Compare September 18, 2023 14:17
@mickel8 mickel8 force-pushed the metrics-http-conf branch 3 times, most recently from c3550fa to 2d249b6 Compare September 18, 2023 18:59
.credo.exs Outdated
@@ -129,7 +129,7 @@
{Credo.Check.Refactor.MatchInCondition, []},
{Credo.Check.Refactor.NegatedConditionsInUnless, []},
{Credo.Check.Refactor.NegatedConditionsWithElse, []},
{Credo.Check.Refactor.Nesting, []},
{Credo.Check.Refactor.Nesting, [max_nesting: 3]},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A little bit more power?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Contributor Author

@mickel8 mickel8 Sep 19, 2023

Choose a reason for hiding this comment

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

We need more power to write a good code :D I belive that 3 leveles of nesting is acceptable. Some functions in ConfigReader uses this

Copy link
Contributor

@Rados13 Rados13 left a comment

Choose a reason for hiding this comment

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

The load balancing test doesn't pass.

config/runtime.exs Show resolved Hide resolved
config/runtime.exs Show resolved Hide resolved
config/prod.exs Outdated Show resolved Hide resolved
.credo.exs Outdated
@@ -129,7 +129,7 @@
{Credo.Check.Refactor.MatchInCondition, []},
{Credo.Check.Refactor.NegatedConditionsInUnless, []},
{Credo.Check.Refactor.NegatedConditionsWithElse, []},
{Credo.Check.Refactor.Nesting, []},
{Credo.Check.Refactor.Nesting, [max_nesting: 3]},
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this?

config/runtime.exs Show resolved Hide resolved
Copy link
Contributor

@Rados13 Rados13 left a comment

Choose a reason for hiding this comment

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

Besides failing load-balancing tests, everything looks fine.

lib/jellyfish/config_reader.ex Outdated Show resolved Hide resolved
def read_port(env) do
if value = System.get_env(env) do
case Integer.parse(value) do
{port, _sufix} when port in 1..65_535 ->
Copy link
Member

Choose a reason for hiding this comment

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

Why is it 1 here and 0 in read_port_range?

lib/jellyfish/config_reader.ex Outdated Show resolved Hide resolved
test/jellyfish/config_reader_test.exs Outdated Show resolved Hide resolved
config/runtime.exs Show resolved Hide resolved
@mickel8 mickel8 requested a review from sgfn September 20, 2023 10:07
Copy link
Member

@sgfn sgfn left a comment

Choose a reason for hiding this comment

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

🥇

@mickel8 mickel8 merged commit 53599d1 into main Sep 20, 2023
2 checks passed
@mickel8 mickel8 deleted the metrics-http-conf branch September 20, 2023 10:21
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.

4 participants