-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
Simplify CNI logging config #51074
Simplify CNI logging config #51074
Conversation
866ff15
to
63e164f
Compare
I think there may be some misunderstand on how the scopes work? Or in my understanding of this change If CNI should use default scope everywhere, why just CNI? That is inconsistent, and we should do the same everywhere. Scopes, though, are useful -- so I don't think we want to d othat
Seems like what you are looking for is |
The main thing is that unless they can be nested and auto inherit the level from the parent scope (which AFAICT they cannot), in this component they're less useful than just labels. We get no real benefit from the segregation, there are no real cases where we'd only want plugin logs, but not ambient informer logs, for instance - that's not a practically useful level of granularity.
They can be, but
I am fine creating a top-level
Yes, but in the
which (if you don't know our logging levels) is confusing - why would anyone know the difference between It sounds like you're saying we should just basically not use |
I still don't get it. If you don't care about scopes then set |
With the current scoping in If no one but people so familiar with the code that they understand the implicit dependencies between the inner components can intelligently make use of the granular scopes, and those people don't use selective granular scopes (I don't, I doubt any other maintainers do for this component) - why do we use scopes vs labels? I can't think of a reason. |
The |
Yes, it is weird. I considered just sending everything and filtering serverside (that is simpler) but that has ~some perf implications? Probably not much. If you wanna have a toggle just on one end, I'll do that. |
If a user doesn't understand the scopes and just wants "all the logs" for debugging, they should not use scopes and should use Why would we remove scopes in CNI and not in all of istio? |
And do we remove them from ztunnel and envoy as well? |
I don't have an opinion on that really, that's IMO strictly out of scope (no pun intended) for this PR - and it's not an argument for why they should be used here, either way. Scopes make much more sense for things in the datapath IMO where you might have or want different log streams with hugely varying volumes that go to different places - access logs + other stuff, basically. CNI isn't in the datapath. It doesn't have logs like that. You either want to trace the flow of pod add/pod remove events thru all the components, or you don't. Labels give context, context is what we need here. We don't have a requirement for sub-component toggling in |
Keeping components of Istio consistent is always in scope 🙂 . |
Fair - for me it comes back to the datapath thing tho - If you definitely want scopes - what scopes would you propose here? The existing scoping is excessively granular, to the point where it is unused. |
Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
63e164f
to
5e7265d
Compare
acb98b3
to
98d5bb4
Compare
Couple of things we can do here (I'm open to any of these):
@howardjohn lmk your thoughts on this, or if there's another option I didn't identify. |
I would do (4), and we at the very least want scopes for the plugin vs the server |
Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
I added those 2 scopes - note tho that the original impetus for this PR was to make the plugin auto-inherit the agent log level, and that doesn't change with this. |
/test integ-security-multicluster |
Please provide a description of this PR:
Fixes #50958
Currently, we use different scopes for all the subcomponents:
which means that
default:XXXX
does not work as you'd expect at all (it's in fact mostly useless), which leads to a lot of confusion when configuringistio-cni
logging levels - you either have to useall:XXX
or know your subcomponents. In practice using separate scopes hurts more than it helps.Also, there's no real reason why we should ask the end-user to separately configure the out-of-process plugin log level, it should just inherit.
This moves all
istio-cni
logging to use thedefault
scope, using labels for the different components, and removes the user-facing separate plugin logging config in favor of inheriting the level from the agent.