Use service environment vars, not setting new vars #248
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
puckpuck
approved these changes
Aug 2, 2022
ahayworth
suggested changes
Aug 2, 2022
|
@mic-max 🦭 |
cartersocha
added a commit
that referenced
this pull request
Aug 3, 2022
This is a follow-up to #248, which in turn was resolving: #242 (comment) The ruby app is relying on convention, and by convention a `PORT` env varable will determine which port the `puma` webserver actually listens on. So - we don't need to set it explicitly in the compose file, but that means we need to set it elsewhere somehow. I tried a few hacks with the dockerfile: - Trying to pass `PORT=$EMAIL_SERVICE_PORT` - Trying to pass `-p $EMAIL_SERVICE_PORT` in the command But those didn't work and I gave up. So instead we just set it directly in the sinatra app ourselves. (Please don't ask me how that actually gets set down all the way through sinatra -> rack -> puma, because truthfully I do not know: rack-based servers are basically magic). We can see that it works: ``` @ahayworth ➜ /workspaces/opentelemetry-demo-webstore (ahayworth/emailservice-port-things ✗) $ git grep EMAIL_SERVICE_P ORT .env:EMAIL_SERVICE_PORT=6060 ``` ``` @ahayworth ➜ /workspaces/opentelemetry-demo-webstore (ahayworth/emailservice-port-things ✗) $ docker-compose up --build emailservice email-service | == Sinatra (v2.2.0) has taken the stage on 6060 for production with backup from Puma email-service | I, [2022-08-03T13:08:27.643291 #1] INFO -- : Instrumentation: OpenTelemetry::Instrumentation::Sinatra was successfully installed with the following options {} email-service | Puma starting in single mode... email-service | * Puma version: 5.6.4 (ruby 3.1.2-p20) ("Birdie's Version") email-service | * Min threads: 0 email-service | * Max threads: 5 email-service | * Environment: production email-service | * PID: 1 email-service | * Listening on http://0.0.0.0:6060 email-service | Use Ctrl-C to stop ``` Co-authored-by: Carter Socha <[email protected]>
jmichalak9
pushed a commit
to jmichalak9/opentelemetry-demo
that referenced
this pull request
Mar 22, 2024
* no extra port env vars * emailservice port undo Co-authored-by: Carter Socha <[email protected]>
jmichalak9
pushed a commit
to jmichalak9/opentelemetry-demo
that referenced
this pull request
Mar 22, 2024
This is a follow-up to open-telemetry#248, which in turn was resolving: open-telemetry#242 (comment) The ruby app is relying on convention, and by convention a `PORT` env varable will determine which port the `puma` webserver actually listens on. So - we don't need to set it explicitly in the compose file, but that means we need to set it elsewhere somehow. I tried a few hacks with the dockerfile: - Trying to pass `PORT=$EMAIL_SERVICE_PORT` - Trying to pass `-p $EMAIL_SERVICE_PORT` in the command But those didn't work and I gave up. So instead we just set it directly in the sinatra app ourselves. (Please don't ask me how that actually gets set down all the way through sinatra -> rack -> puma, because truthfully I do not know: rack-based servers are basically magic). We can see that it works: ``` @ahayworth ➜ /workspaces/opentelemetry-demo-webstore (ahayworth/emailservice-port-things ✗) $ git grep EMAIL_SERVICE_P ORT .env:EMAIL_SERVICE_PORT=6060 ``` ``` @ahayworth ➜ /workspaces/opentelemetry-demo-webstore (ahayworth/emailservice-port-things ✗) $ docker-compose up --build emailservice email-service | == Sinatra (v2.2.0) has taken the stage on 6060 for production with backup from Puma email-service | I, [2022-08-03T13:08:27.643291 open-telemetry#1] INFO -- : Instrumentation: OpenTelemetry::Instrumentation::Sinatra was successfully installed with the following options {} email-service | Puma starting in single mode... email-service | * Puma version: 5.6.4 (ruby 3.1.2-p20) ("Birdie's Version") email-service | * Min threads: 0 email-service | * Max threads: 5 email-service | * Environment: production email-service | * PID: 1 email-service | * Listening on http://0.0.0.0:6060 email-service | Use Ctrl-C to stop ``` Co-authored-by: Carter Socha <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #242 (comment)
Changes
Services will now read their port environment variables directly rather than having any intermediary $PORT variable for example.
Also, getting any environment variables should be done at startup of the service and if it is not present it can crash the program with a relevant error that would be more helpful than starting the service using a default port number that might not be actually communicating with other services which would be harder to detect problems.