BentoML
BentoML copied to clipboard
bentoml
fix(monitoring): don't override user set environment variables
What does this PR address?
This PR is stacked on top of #4808
It shocked me that this bit of code sets environment variables! Does this not make it possible for monitoring and tracing configuration to conflict?
https://github.com/bentoml/BentoML/pull/3257#discussion_r1639956769
Before submitting:
- [ ] Does the Pull Request follow Conventional Commits specification naming? Here are GitHub's guide on how to create a pull request.
- [ ] Does the code follow BentoML's code style,
pre-commit run -ascript has passed (instructions)? - [ ] Did you read through contribution guidelines and follow development guidelines?
- [ ] Did your changes require updates to the documentation? Have you updated those accordingly? Here are documentation guidelines and tips on writting docs.
- [ ] Did you write tests to cover your changes?
no, we need to set via environment for controlling on kubernetes.
Why? Is that specific to BentoCloud because the user doesn't actually have control over the Pod's spec? Otherwise, the user should still be responsible for setting the environment variables of their own Pods? I'm confused because we're running on Kubernetes and specifically don't want these env vars overridden.
I'm find with having hierarchical to check for env -> variable
That still strikes me as leading so some slightly odd behaviour. For example, imagine you want to send traces and logs to two different OTLP Collectors (as I do). I set the environment variables match the endpoint I want to send the traces to and I configure the logs via code in the bentoml.service decorator. Where do my traces go? 🤷♂️ I genuinely don't know the answer but it seems very confusing and delicate and could change unexpectedly given that the code is sometimes manually overriding my environment variables but sometimes not.
At the very very very least there should probably be something either as a comment in the code or in the docs explaining what the behaviour is.
gPRC exporter seems to be a separate PR.
True... but also some of the arguments and environment variables being set literally do nothing so I'd almost argue that support the GRPC Exporter is a fix rather than a feature addition 😆 insecure for example seems to only be relevant for the GRPC exporter. Happy to split it out but surprised it made it through code review.
no, we need to set via environment for controlling on kubernetes.
Alternatively, should we be setting only the OTEL_EXPORTER_OTLP_LOGS_* variables? Still not sure setting these at all is great. But it'd be better.
