Skip to content
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

fix(monitoring): don't override user set environment variables #4807

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

judahrand
Copy link
Contributor

@judahrand judahrand commented Jun 14, 2024

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?

#3257 (comment)

Before submitting:

@judahrand judahrand requested a review from a team as a code owner June 14, 2024 13:39
@judahrand judahrand requested review from sauyon and removed request for a team June 14, 2024 13:39
Copy link
Contributor

@aarnphm aarnphm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, we need to set via environment for controlling on kubernetes.

I'm find with having hierarchical to check for env -> variable

gPRC exporter seems to be a separate PR.

@judahrand
Copy link
Contributor Author

judahrand commented Jun 15, 2024

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.

@judahrand
Copy link
Contributor Author

judahrand commented Jun 15, 2024

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.

@judahrand
Copy link
Contributor Author

judahrand commented Jun 15, 2024

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.

@judahrand judahrand changed the title feat(monitoring): don't set environment variables and allow use of GRPC OTLPLogExporter fix(monitoring): don't override user set environment variables Jun 15, 2024
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.

2 participants