Skip to content

Conversation

@Kami
Copy link
Member

@Kami Kami commented Jun 27, 2018

This pull request installs gunicorn worker logging config files and updates service manager files to pass --logging-config argument to the gunicorn process.

For context and details, see StackStorm/st2#4206.

API based services.

This way we don't miss various log messages generated by gunicorn worker
processes.

Corresponding StackStorm/st2 change: StackStorm/st2#4206
@arm4b
Copy link
Member

arm4b commented Jun 27, 2018

While the change is a generally good thing that fixes big, I have a few questions:

How st2api/conf/logging.gunicorn.conf is different from st2api/conf/logging.conf?
When one is used and when another? I'm curious if st2.conf logging settings we set make any sense.
Which part of app is logged via gunicorn config and which part via old logging config?

Is it possible to have one logging config for services like api stream auth, instead of two configs?

At this moment sounds like to change the service to log to stdout we'll need to edit 2 files?

@Kami
Copy link
Member Author

Kami commented Jun 27, 2018

Responded in Slack, to recap:

We can't re-use logging config file from a st2 service directly because StackStorm services inject new AUDIT log level which is not known to gunicorn workers so it would break if we used st2 logging config file directly for gunicorn (in fact, that's was my first attempt - trying to re-use the same configs for gunicorn).

We could perhaps try to do some deduplication and move some of duplicated stuff to build time. This would perhaps decrease duplication a bit, but it would make it more opaque (harder to see where things are set and changed).

@Kami
Copy link
Member Author

Kami commented Jun 27, 2018

Note: Circle CI build will keep failing until st2 changes are merged into master.

Copy link
Member

@arm4b arm4b 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 explanation.
Overall it's kind of confusing for any user, while looking at st2.conf logging settings and expecting to change things in a single place.

But if there is no other way to do it, - LGTM as this PR fixes particular bug.

@Kami Kami merged commit a301d03 into master Jun 27, 2018
@Kami Kami deleted the add_gunicorn_logging_configs branch June 27, 2018 17:15
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.

3 participants