Hi Alex, thanks for being flexible. Given how valuable the tag is for day-to-day triage, I’d prefer an opt-out model:
1. Default is enabled, so that operators get realm-level visibility “for free.” 2. Anyone worried about cardinality can turn it off with a single flag (metrics.realm-id.enabled=false). That keeps the out-of-box experience solid for multi-tenant clusters while still giving freedom to strip the label in edge cases. Happy to review the PR once you flip it to opt-out. Yufei On Sat, May 24, 2025 at 3:42 AM Alex Dutra <alex.du...@dremio.com.invalid> wrote: > Hi Yufei, > > Thanks for sharing your concerns. > > In fact we were already leaning towards an enablement flag, however > from Mike's message I inferred that the metric tag would be an opt-in > feature, but you seem to suggest an opt-out feature instead. > > I'm fine either way, the PR I opened uses opt-in, but I can change to > opt-out. Let's resolve this detail in the PR. > > Thanks, > > Alex > > On Sat, May 24, 2025 at 12:22 AM Yufei Gu <flyrain...@gmail.com> wrote: > > > > Hey folks, > > > > I’m not sold on dropping realm_id. > > > > Why it matters > > > > 1. It's pretty useful for first-line triage in multi-tenant clusters. > > When a latency spike or error burst hits the dashboards, the first SRE > > question is “Which realm(s)?” Having the tag lets us slice the > time-series > > instantly instead of hopping tools or waiting for sampled traces. > > 2. Cardinality is bounded in practice. Even big deployments seldom > break > > a few dozen active realms per Polaris instance. If someone configures > > “hundreds,” that’s an intentional choice we should surface and > monitor. > > Some TSDBs scale just fine in that situation. > > 3. Metrics and traces serve different loops. Traces are for deep-dive > > forensics once you have a suspect request; metrics drive real-time > alerting > > and capacity planning. Relying only on traces every on-call to see > which > > tenant is noisy. > > > > If we really need a safety valve, add a boolean flag like > > "metrics.realm-id.enabled", default true. Operators who don’t want the > > dimension can flip it off; everyone else keeps the out-of-the-box > > visibility that keeps multi-tenant ops sane. > > Dropping the tag outright trades day-to-day operability for a theoretical > > scaling worry most users will never hit. Let’s keep it and offer an > opt-out > > instead. > > > > Yufei > > > > > > On Fri, May 23, 2025 at 5:49 AM Alex Dutra <alex.du...@dremio.com.invalid > > > > wrote: > > > > > Hi, > > > > > > For reference the PR is here: > https://github.com/apache/polaris/pull/1662 > > > > > > Alex > > > > > > On Fri, May 23, 2025 at 2:42 PM Robert Stupp <sn...@snazy.de> wrote: > > > > > > > > +1 on adding a config and default to false - and later entirely > remove > > > > the "issue". > > > > > > > > Sounds like good plan to me! > > > > > > > > On 23.05.25 13:52, Alex Dutra wrote: > > > > > Hi Mike, > > > > > > > > > > That sounds like a plan! And yes, I agree with the slow approach. I > > > > > will open a PR shortly with the new enablement flag. > > > > > > > > > > Alex > > > > > > > > > > On Fri, May 23, 2025 at 2:42 AM Michael Collado < > > > collado.m...@gmail.com> wrote: > > > > >> My immediate reaction is that a configuration flag would be good > > > enough for > > > > >> us. Our setup prevents requests without a valid realm, so we're > not > > > subject > > > > >> to the DDOS attack you mentioned, but we should address that in > the > > > default > > > > >> realm context resolver as well. I can do some further > investigation > > > to see > > > > >> if it's something we could get rid of entirely (we also use > log-based > > > > >> metrics), but I like the slow approach of adding a configuration > flag, > > > > >> disabling it by default, then removing functionality as a best > > > practice, > > > > >> anyway. > > > > >> > > > > >> Mike > > > > >> > > > > >> On Thu, May 22, 2025 at 11:39 AM Alex Dutra > > > <alex.du...@dremio.com.invalid> > > > > >> wrote: > > > > >> > > > > >>> Hi again, > > > > >>> > > > > >>> Let's go back to the main concern: excessive memory required for > > > metrics. > > > > >>> > > > > >>> To move this discussion forward, would you all agree to > introduce a > > > > >>> flag that activates the inclusion of realm IDs in the metrics? > > > > >>> > > > > >>> It seems like this could be a good middle ground that addresses > > > > >>> everyone's needs. > > > > >>> > > > > >>> Let me know your thoughts. > > > > >>> > > > > >>> Alex > > > > >>> > > > > >>> > > > > >>> On Wed, May 21, 2025 at 3:57 PM Robert Stupp <sn...@snazy.de> > wrote: > > > > >>>> Ouch! That sounds very simple to exploit and quite serious. > > > > >>>> > > > > >>>> On 21.05.25 15:38, Alex Dutra wrote: > > > > >>>>> Also worth noting: http_server_requests_seconds_* metrics are > > > generated > > > > >>>>> even for failed requests. So it would be very easy for an > attacker > > > to > > > > >>> forge > > > > >>>>> HTTP requests containing invalid realms and take the server > down. > > > > >>>>> > > > > >>>>> Alex > > > > >>>>> > > > > >>>>> On Wed, May 21, 2025 at 3:04 PM Robert Stupp <sn...@snazy.de> > > > wrote: > > > > >>>>> > > > > >>>>>> OOMs are bad - it's extremely hard (if not impossible) for any > > > user to > > > > >>>>>> figure out why that happens. Bug reports would just read like > > > "Polaris > > > > >>>>>> runs into an OOM - no further information available". > > > > >>>>>> > > > > >>>>>> IMHO the system should be stable by default, not the other way > > > around. > > > > >>>>>> Even if there is a way to enable such huge cardinalities, that > > > flag > > > > >>>>>> should really be hidden and not documented anywhere - unless > > > there's a > > > > >>>>>> clear disclaimer saying: "Enabling this can lead to an > > > unresponsive > > > > >>>>>> system and risk the stability of the system. ..." > > > > >>>>>> > > > > >>>>>> I propose to investigate all metrics and reduce the > cardinality > > > of all > > > > >>>>>> of those as that can easily become a serious issue. > > > > >>>>>> > > > > >>>>>> On 21.05.25 11:42, Alex Dutra wrote: > > > > >>>>>>> Hi Mike, > > > > >>>>>>> > > > > >>>>>>> If you have around a hundred realms or more, imho you are > > > already in > > > > >>>>>>> trouble. > > > > >>>>>>> > > > > >>>>>>> The most problematic metric is the > http_server_requests_seconds_* > > > > >>> metrics > > > > >>>>>>> family, e.g.: > > > > >>>>>>> > > > > >>>>>>> > > > > >>> > > > > http_server_requests_seconds_count{application="Polaris",method="GET",outcome="CLIENT_ERROR",realm_id="POLARIS",status="404",uri="NOT_FOUND",} > > > > >>>>>>> 1.0 > > > > >>>>>>> > > > > >>>>>>> Since metrics in this family already have a high cardinality > > > > >>> potential > > > > >>>>>>> given the number of tags it supports by default, adding one > more > > > > >>>>>> dimension > > > > >>>>>>> to them makes things (exponentially) worse. > > > > >>>>>>> > > > > >>>>>>> It's very easy to demonstrate that by running a small test > [1]. > > > On my > > > > >>>>>>> machine, the first 2 iterations (10 and 100 realms) > complete, but > > > > >>> the 3rd > > > > >>>>>>> iteration (1000 realms) runs for about 1 minute then ends up > in > > > > >>>>>>> java.lang.OutOfMemoryError: Java heap space. > > > > >>>>>>> > > > > >>>>>>> That's why I advocated for removing the tag. If however you > > > really > > > > >>> want > > > > >>>>>> to > > > > >>>>>>> keep it, I'd suggest introducing a configuration flag to > disable > > > it > > > > >>> in > > > > >>>>>> two > > > > >>>>>>> problematic metric families: the HTTP one shown above, and > the > > > > >>>>>> per-endpoint > > > > >>>>>>> metrics as well. > > > > >>>>>>> > > > > >>>>>>> Thanks, > > > > >>>>>>> > > > > >>>>>>> Alex > > > > >>>>>>> > > > > >>>>>>> [1]: > > > https://gist.github.com/adutra/414fe773e8727304b34e9249299c988d > > > > >>>>>>> > > > > >>>>>>> > > > > >>>>>>> > > > > >>>>>>> On Wed, May 21, 2025 at 7:35 AM Michael Collado < > > > > >>> collado.m...@gmail.com> > > > > >>>>>>> wrote: > > > > >>>>>>> > > > > >>>>>>>> Hmm, we do use the realm tag in our metric publishing. I > > > understand > > > > >>> the > > > > >>>>>>>> concern re: cardinality. Maybe we can support filtering > metrics > > > that > > > > >>>>>> have > > > > >>>>>>>> realm and support another metric without realm? > > > > >>>>>>>> > > > > >>>>>>>> On Mon, May 19, 2025 at 12:24 PM Dmitri Bourlatchkov < > > > > >>> di...@apache.org> > > > > >>>>>>>> wrote: > > > > >>>>>>>> > > > > >>>>>>>>> Removing realm_id from metrics tags makes sense to me (to > avoid > > > > >>> high > > > > >>>>>>>>> cardinality). > > > > >>>>>>>>> > > > > >>>>>>>>> If we need to have insight into load differences from > realm to > > > > >>> realm, > > > > >>>>>> it > > > > >>>>>>>>> might be preferable to introduce metrics dedicated to that > > > rather > > > > >>> than > > > > >>>>>>>>> increasing the cardinality of every endpoint metric. > > > > >>>>>>>>> > > > > >>>>>>>>> Cheers, > > > > >>>>>>>>> Dmitri. > > > > >>>>>>>>> > > > > >>>>>>>>> On Thu, May 15, 2025 at 3:30 PM Alex Dutra > > > > >>>>>> <alex.du...@dremio.com.invalid > > > > >>>>>>>>> wrote: > > > > >>>>>>>>> > > > > >>>>>>>>>> Hi all, > > > > >>>>>>>>>> > > > > >>>>>>>>>> I would like to suggest removing the "realm_id" metric tag > > > > >>> entirely. > > > > >>>>>>>>>> My concern is that this tag has the potential for high > > > > >>> cardinality, > > > > >>>>>>>> which > > > > >>>>>>>>>> is generally considered a bad practice when dealing with > > > metrics. > > > > >>> High > > > > >>>>>>>>>> cardinality can lead to performance issues and increased > > > memory > > > > >>> usage. > > > > >>>>>>>>>> Granted, the default realm resolver in Polaris is > tailored for > > > > >>> just a > > > > >>>>>>>>>> handful of realms, but nothing prevents users from > declaring > > > > >>> hundreds > > > > >>>>>>>> of > > > > >>>>>>>>>> realms. > > > > >>>>>>>>>> > > > > >>>>>>>>>> I believe we can still effectively monitor Polaris servers > > > without > > > > >>>>>> this > > > > >>>>>>>>>> specific tag, since the realm ID is also propagated in > traces > > > > >>> emitted > > > > >>>>>>>> by > > > > >>>>>>>>>> Polaris. Tracing is a much better fit for high-cardinality > > > > >>> domains. > > > > >>>>>>>>>> I'm open to discussing this further; a potential > alternative > > > > >>> would be > > > > >>>>>>>> to > > > > >>>>>>>>>> introduce a flag to disable this specific metric tag, but > I > > > feel > > > > >>> like > > > > >>>>>>>>>> removing it would be a much cleaner approach. > > > > >>>>>>>>>> > > > > >>>>>>>>>> Let me know your thoughts. > > > > >>>>>>>>>> > > > > >>>>>>>>>> Thanks, > > > > >>>>>>>>>> > > > > >>>>>>>>>> Alex > > > > >>>>>>>>>> > > > > >>>>>> -- > > > > >>>>>> Robert Stupp > > > > >>>>>> @snazy > > > > >>>>>> > > > > >>>>>> > > > > >>>> -- > > > > >>>> Robert Stupp > > > > >>>> @snazy > > > > >>>> > > > > -- > > > > Robert Stupp > > > > @snazy > > > > > > > >
