-
Notifications
You must be signed in to change notification settings - Fork 6.5k
add microservices demo #509
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
add microservices demo #509
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
Signed CLA |
CLAs look good, thanks! |
Any changes needed here? |
Hey @travisoneill apologies for the long response here - I'm at a team summit but will be able to review & merge this next week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished a pass, let me know when you're ready for another review.
@@ -0,0 +1,23 @@ | |||
# Ignore bundler config. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file shouldn't be needed anymore, right?
@@ -0,0 +1,80 @@ | |||
# Python Google Cloud Microservices Example - API Gateway |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd actually prefer if we put this sample in a subdirectory called multiple_services
.
@@ -0,0 +1,80 @@ | |||
# Python Google Cloud Microservices Example - API Gateway | |||
|
|||
This example demonstrates how to deploy multiple python services to [App Engine Flexible Environment](https://cloud.google.com/appengine/docs/flexible/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just 'flexible environment` (no caps).
5. To run each server locally: | ||
```Bash | ||
$ source env/bin/activate | ||
$ export FLASK_DEBUG=1 #optional to run in debug mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't need this.
|
||
Each directory contains an `app.yaml file`. This file will be the same as if | ||
each service were its own project except for the `service` line. For the gateway: | ||
```YAML |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just link to the files, no need to include them here. (They'll inevitably get out of sync)
@app.route('/hello/<service>') | ||
def say_hello(service): | ||
services = { | ||
'flask1': { 'url': 'https://flask1-dot-flask-algo.appspot.com/', 'send': False }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard-coded urls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonparrott The URLs will depend on the project ID of when the user deploys the demo, so he will have to fill those in. (Unless there is another way to handle that?) I could just leave a comment there as a placeholder and add something in the readme to instruct the user to enter his urls there. Would that be good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an environment variable I think you can use: https://cloud.google.com/appengine/docs/flexible/python/runtime#environment_variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonparrott It doesn't seem to like the environment variable. I'm using os.environ['GAE_APPENGINE_HOSTNAME']
. Getting 500's and I have now added print(os.environ)
to see what I am working with but can't seem to find the logs. I am getting the logged requests, but not able to see the actual error messages or logs from my print
command. Do you know where I would go to access this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@travisoneill they should be logged under the stdout
log in the cloud console.
If I understand correctly, we're making some changes to the environment variables. It might be better to just do something like this:
PROJECT_ID = os.environ.get('GAE_LONG_APP_ID')
DEFAULT_MODULE_URL = '{}.appspot.com'.format(PROJECT_ID)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonparrott Already done. I constructed the urls with GAE_LONG_APP_ID
. Made your other changes as well and ready for review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we could still get the point across of this sample without having 4 separate services. Can we reduce it to 2 or 3?
@@ -0,0 +1,10 @@ | |||
# Ignore python virtual environment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, are these not all already a part of the top-level .gitignore?
|
||
This example demonstrates how to deploy multiple python services to [App Engine flexible environment](https://cloud.google.com/appengine/docs/flexible/) | ||
|
||
## To Run Locally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This local run story is pretty rough, and some of that blame of course falls on Google. Do you have any ideas on how we can improve this for this sample (procfile + honcho)? Overall?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonparrott I've got it running now so that it will work as previously in production, and the user can run locally by using a --development
flag from the command line without having to do anything with the code. Is that sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable, push the code up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Needs the other changes though, and testing with deployed code. Will push up now so you can see and again when changes complete.
### YAML Files | ||
|
||
Each directory contains an `app.yaml file`. This file will be the same as if | ||
each service were its own project except for the `service` line. For the gateway: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to remove the line "This file will be the same as if
+each service were its own project except for the service
line.", perhaps "These files all describe a separate App Engine service within the same project".
|
||
runtime_config: | ||
python_version: 3 | ||
manual_scaling: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blank newline above this, please.
@@ -0,0 +1,11 @@ | |||
# [START runtime] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for these fences (We'll add them when/if we add this to the docs)
@jonparrott I was basing it off my actual app structure, but there is no need for it to be that many. I could cut it down to 3, or even 2 if I only had the static file server behind the gateway. |
@travisoneill let's do that, simple is better. Our mantra is "do the simplest thing that works". |
@@ -12,7 +12,7 @@ def root(): | |||
return 'This is flask server 1.' | |||
|
|||
if __name__ == "__main__": | |||
if len(sys.argv) > 1 and sys.argv[1] == 'development': | |||
if len(sys.argv) > 1 and sys.argv[1] == '--development': | |||
app.run(port=int(8001)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debug=True
?
@@ -12,7 +12,7 @@ def root(): | |||
return 'This is flask server 1.' | |||
|
|||
if __name__ == "__main__": | |||
if len(sys.argv) > 1 and sys.argv[1] == 'development': | |||
if len(sys.argv) > 1 and sys.argv[1] == '--development': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How awful would it be to introduce argparse here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also make the port an arg. :)
@app.route('/environment') | ||
def get_env(): | ||
return str(os.environ) | ||
configuration = 'production' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused variable?
'production': 'https://static-dot-' + os.environ.get('GAE_LONG_APP_ID', '') + '.appspot.com' | ||
} | ||
environment = app.config['MODE'] | ||
print(environment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leftover prints?
print('https://static-dot-' + os.environ['GAE_LONG_APP_ID'] + '.appspot.com') | ||
# return 'https://static-dot-' + os.environ['GAE_LONG_APP_ID'] + '.appspot.com' | ||
res = requests.get('https://static-dot-' + os.environ['GAE_LONG_APP_ID'] + '.appspot.com') | ||
urls = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could make this a constant?
@jonparrott Made this before I saw your comments. Would be happy to add argparse if you think its necessary. |
Please do. :) |
@app.route('/test') | ||
def test(): | ||
return "GATEWAY OPERATIONAL" | ||
print(app.config['SERVICE_MAP']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stray print?
def test(): | ||
return "GATEWAY OPERATIONAL" | ||
print(app.config['SERVICE_MAP']) | ||
return app.config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
returning the entire app.config can be a security issue, can you limit the scope here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Meant to take that out that was just for me to play around with!
argparser added and readme updated to reflect changes in the structure of the app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking close!
Each directory contains an `app.yaml` file. These files all describe a | ||
separate App Engine service within the same project. | ||
|
||
For the gateway: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use gcloud to deploy multiple services at once:
gcloud app deploy gateway/app.yml static/app.yml
#create service map for production urls unless --development flag added | ||
app.config['SERVICE_MAP'] = services_config.map_services('production') | ||
|
||
#setup arg parser to handle development flag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stick this down in the if __main__
section.
def production_url(service_name): | ||
project_id = os.environ.get('GAE_LONG_APP_ID') or 'flask-algo' | ||
project_url = project_id + '.appspot.com' | ||
template = 'https://{}{}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use named interpolation if you use a template more than once, e.g. https://{service_name_prefix}{project_url}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Named interpolation was really verbose so I removed the template.
if service_name == 'default':
return 'https://{}'.format(project_url)
else:
return 'https://{}-dot-{}'.format(service_name, project_url)
return template.format(service_name + '-dot-', project_url) | ||
|
||
def local_url(port): | ||
return 'http://localhost:' + str(port) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use format instead of concatenation.
return app.send_static_file(path), 200, {'Content-Type': mimetype} | ||
|
||
if __name__ == "__main__": | ||
if len(sys.argv) > 1 and sys.argv[1] == '--development': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
argparse?
Can't put the production config in the Have you guys thought of adding an environment variable that specifies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@travisoneill I need you to add license headers to all of the .py .js, and .html files before I can merge this.
if args.development: | ||
app.config['SERVICE_MAP'] = services_config.map_services('development') | ||
app.run() | ||
port = os.environ.get('PORT') or 8000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't this error because PORT
is a string?
|
||
def make_app(name): | ||
app = Flask(name) | ||
environment = 'production' if os.environ.get('GAE_LONG_APP_ID') else 'development' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the time being, I recommend using os.environ.get('GAE_INSTANCE', os.environ.get('GAE_MODULE_INSTANCE'))
.
@jonparrott Changes made. Thanks for all the code reviews! Do you want a similar sample app for the Node environment? |
@travisoneill that's up to @jmdobry. :) Thanks for your contribution! |
add code line to set metric label in addition to resource label. make the code sample align with similar samples in other languages.
add code line to set metric label in addition to resource label. make the code sample align with similar samples in other languages.
PR from issue #499