Added helmchart variable to operator to be able to set a default "global" request logging prefix #1517

axsaucedo · 2020-03-07T11:48:22Z

Fixes #1510

This PR includes:

Added extra env variable to executor to add a default request logging prefix
Added changes in executor to use this as default variable
Default env var is cached in executor code with variable to avoid accessing getenv for efficiency
Helmchart resources python file also has the extra env variable for helmchart generation
Helmchart resources python file was edited by formatter (hence why all file is updated)

Things to consider:

Currently it allows for the prefix to be overriden, which still assumes that there will be one request logger per namespace. If we think it's best to make it a REQUEST_LOGGER_ENDPOINT instead of REQUEST_LOGGER_ENDPOINT_PREFIX then we'd just need to make sure it reflects this.
Currently the changes made the ENV variable to be optional as there is a "default" value in executor/logger/constants.go, however if we add this value as the default when the executor container is being created in the operator we would be abel to have that default there instead.

Further questions:

Currently the way to enable request logging needs to be set on every deployment, for every predictor. It could also be possible to enable request logging on a global level, with the ability to enable/disable on a per-deployment level. If this is something desirable, then we would look at adding another variable and ensuring that it's checked in the executor's predictor_process Predict method.

…logging prefix

REmoved previous edits of types

seldondev · 2020-03-07T11:49:39Z

Sat Mar 7 11:49:38 UTC 2020
The logs for [lint] [2] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-1517/2.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-1517 --build=2

seldondev · 2020-03-07T11:49:39Z

Sat Mar 7 11:49:38 UTC 2020
The logs for [pr-build] [1] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-1517/1.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-1517 --build=1

seldondev · 2020-03-07T11:50:40Z

Sat Mar 7 11:50:39 UTC 2020
The logs for [pr-build] [3] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-1517/3.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-1517 --build=3

seldondev · 2020-03-07T11:50:47Z

Sat Mar 7 11:50:46 UTC 2020
The logs for [lint] [4] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-1517/4.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-1517 --build=4

…github.com/axsaucedo/seldon-core into 1510_add_top_level_request_logger_registry

seldondev · 2020-03-07T11:59:19Z

Sat Mar 7 11:59:18 UTC 2020
The logs for [lint] [6] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-1517/6.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-1517 --build=6

seldondev · 2020-03-07T11:59:40Z

Sat Mar 7 11:59:40 UTC 2020
The logs for [pr-build] [5] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-1517/5.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-1517 --build=5

axsaucedo · 2020-03-07T12:04:39Z

Questions to consider:

Currently the way to enable request logging needs to be set on every deployment, for every predictor. It could also be possible to enable request logging on a global level, with the ability to enable/disable on a per-deployment level. If this is something desirable, then we would look at adding another variable and ensuring that it's checked in the executor's predictor_process Predict method.

axsaucedo · 2020-03-07T12:16:01Z

Another thing to consider:
Currently it allows for the prefix to be overriden, which still assumes that there will be one request logger per namespace. If we think it's best to make it a REQUEST_LOGGER_ENDPOINT instead of REQUEST_LOGGER_ENDPOINT_PREFIX then we'd just need to make sure it reflects this.

ryandawsonuk · 2020-03-07T13:32:44Z

Just thinking about the use an env var. I realise I suggested this but executor actually tends to use startup parameters instead.

Just a detail though. I think we should look to get something in and refine later.

axsaucedo · 2020-03-07T14:14:36Z

No, that's definitely a good point. I think from a design perspective using parameters would be cleaner, but there wasn't any obvious way of adding it without some bigger changes. If you think we should do it in a different way, we could add it through the param and have the default URL as part of the PredictorProcess class, or alternatively we could have it as an env variable for now/initially and explore the best way to manage the config.

ryandawsonuk · 2020-03-09T10:03:30Z

I take it with this change we'd only need to set the env var at operator install time and then on each SeldonDeployment we'd turn on logging with e.g.

      logger:
        mode: all

axsaucedo · 2020-03-09T13:45:33Z

@ryandawsonuk that is correct - I was thinking that it may be good to add that as a default, but that could be something as a follow-up

axsaucedo · 2020-03-09T13:52:45Z

/test integration

seldondev · 2020-03-09T13:53:51Z

Mon Mar 9 13:53:50 UTC 2020
The logs for [integration] [7] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-1517/7.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-1517 --build=7

ryandawsonuk

/lgtm
/approve

seldondev · 2020-03-09T13:58:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ryandawsonuk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ryandawsonuk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

seldondev · 2020-03-09T15:31:43Z

@axsaucedo: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
integration	`4702272`	link	`/test integration`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

seldondev · 2020-03-09T19:35:05Z

New changes are detected. LGTM label has been removed.

seldondev · 2020-03-09T19:38:01Z

Mon Mar 9 19:37:57 UTC 2020
The logs for [pr-build] [8] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-1517/8.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-1517 --build=8

seldondev · 2020-03-09T19:38:02Z

Mon Mar 9 19:37:57 UTC 2020
The logs for [lint] [9] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-1517/9.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-1517 --build=9

Added extra environment variable to be able to set a default request …

a3b30d9

…logging prefix

axsaucedo requested a review from ukclivecox March 7, 2020 11:48

seldondev added the size/L label Mar 7, 2020

Update seldondeployment_types.go

9782a30

REmoved previous edits of types

axsaucedo added 2 commits March 7, 2020 11:57

Simplified the variable assignment

b565a82

Merge branch '1510_add_top_level_request_logger_registry' of https://…

4702272

…github.com/axsaucedo/seldon-core into 1510_add_top_level_request_logger_registry

axsaucedo requested review from gsunner and ryandawsonuk March 9, 2020 08:09

ryandawsonuk approved these changes Mar 9, 2020

View reviewed changes

seldondev assigned ryandawsonuk Mar 9, 2020

seldondev added the lgtm label Mar 9, 2020

seldondev added the approved label Mar 9, 2020

Added default broker in helm chart

9cd19a6

seldondev removed the lgtm label Mar 9, 2020

seldondev merged commit 9ff9647 into SeldonIO:master Mar 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added helmchart variable to operator to be able to set a default "global" request logging prefix #1517

Added helmchart variable to operator to be able to set a default "global" request logging prefix #1517

axsaucedo commented Mar 7, 2020 •

edited

Loading

seldondev commented Mar 7, 2020

seldondev commented Mar 7, 2020

seldondev commented Mar 7, 2020

seldondev commented Mar 7, 2020

seldondev commented Mar 7, 2020

seldondev commented Mar 7, 2020

axsaucedo commented Mar 7, 2020

axsaucedo commented Mar 7, 2020

ryandawsonuk commented Mar 7, 2020

axsaucedo commented Mar 7, 2020

ryandawsonuk commented Mar 9, 2020

axsaucedo commented Mar 9, 2020

axsaucedo commented Mar 9, 2020

seldondev commented Mar 9, 2020

ryandawsonuk left a comment

seldondev commented Mar 9, 2020

seldondev commented Mar 9, 2020

seldondev commented Mar 9, 2020

seldondev commented Mar 9, 2020

seldondev commented Mar 9, 2020

Added helmchart variable to operator to be able to set a default "global" request logging prefix #1517

Added helmchart variable to operator to be able to set a default "global" request logging prefix #1517

Conversation

axsaucedo commented Mar 7, 2020 • edited Loading

seldondev commented Mar 7, 2020

seldondev commented Mar 7, 2020

seldondev commented Mar 7, 2020

seldondev commented Mar 7, 2020

seldondev commented Mar 7, 2020

seldondev commented Mar 7, 2020

axsaucedo commented Mar 7, 2020

axsaucedo commented Mar 7, 2020

ryandawsonuk commented Mar 7, 2020

axsaucedo commented Mar 7, 2020

ryandawsonuk commented Mar 9, 2020

axsaucedo commented Mar 9, 2020

axsaucedo commented Mar 9, 2020

seldondev commented Mar 9, 2020

ryandawsonuk left a comment

Choose a reason for hiding this comment

seldondev commented Mar 9, 2020

seldondev commented Mar 9, 2020

seldondev commented Mar 9, 2020

seldondev commented Mar 9, 2020

seldondev commented Mar 9, 2020

axsaucedo commented Mar 7, 2020 •

edited

Loading