I agree with your point, too, Asaf. Thanks for posting it on the mailing list.
- Michael
On Mon, Nov 7, 2022 at 6:38 PM PengHui Li <codelipeng...@gmail.com> wrote:
>
> Hi Asaf,
>
> Agree with your point.
> +1 for the new indicator name solution.
>
> Penghui
>
> > On Nov 7, 2022, at 17:26, Asaf Mesika <asaf.mes...@gmail.com> wrote:
> >
> > I'm reposting my comment from the PR as this requires further discussion to
> > reach an agreement.
> >
> > -------
> >
> > You have decided to re-use the same metric name, but with removing the
> > namespace dimension. So we would do something like this in case we turn on
> > topic-level metrics:
> >
> > pulsar_rate_in{host="brokerA"} - broker-level rate in
> > pulsar_rate_in{host="brokerA" namespace="ns1" topic="topic10"} - topic
> > level rate in
> >
> > This creates duplicates when users will use it.
> > Say you wanted to see your cluster rate in, you would do something like
> > sum(pulsar_rate_in), but this sum two things:
> >
> > 1. Topic level rate in, across all brokers.
> > 2. Broker level rate in, across all brokers
> >
> > So, in effect, the rate would be doubled.
> >
> > The idea is that the metric is divided into one granularity level, be it
> > (namespace, topic) or (namespace) so that sum by any dimension they wish.
> > You can't report two granularity levels of any metric under the same name.
> >
> > Your previous version of adding another metric name and explicitly stating
> > its broker-level metric pulsar_broker_rate_in makes more sense.
> >
> > IMO, it must be changed back, unless I'm missing something.
> >
> > ---------
> >
> >
> > Yang, please note that doing what you suggested
> >
> >
> > If the user uses the following query, there will be no double rate:
> >> sum(pulsar_rate_in{namespace=""})
> >
> >
> >
> > IMO is not a valid user experience for a Pulsar user. People don't do this
> > stuff (attribute=""), and we don't expect them to know this small detail
> > when authoring queries.
> >
> >
> > In theory, this should not be the concern of Pulsar at all - making the
> > aggregation since it can be done using TSDB like Prometheus recording rules
> > - but I do understand that emitting so many time series is inherently a
> > Pulsar issue. I accept this as a workaround for now.
> >
> > I hope the work researched here
> > <https://lists.apache.org/thread/comd0o5760fcgc8qn5d0s7bs7c3zs1j3> will
> > yield a better long-term solution.
> >
> >
> > Thanks,
> >
> >
> > Asaf
> >
> > On Mon, Oct 24, 2022 at 10:06 PM Michael Marshall <mmarsh...@apache.org>
> > wrote:
> >
> >> I think this feature is already available out of the box, as Penghui
> >> suggests. You can get metrics per broker summing them up over the
> >> `instance` label or the `kubernetes_pod_name` label. These labels are
> >> added not by the broker, but instead by the prometheus scrape
> >> definition.
> >>
> >> Also, you are correct that there are some metrics that are always 0, e.g.:
> >>
> >> pulsar_topics_count{cluster="my-cluster"} 0
> >>
> >> The motivation for those metrics is provided here [0], though I'm not
> >> sure it is good motivation.
> >>
> >> Thanks,
> >> Michael
> >>
> >> [0]
> >> https://github.com/apache/pulsar/blob/6d6665e296e6714801048dee1292e3a07abb5cc1/pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/prometheus/NamespaceStatsAggregator.java#L306-L307
> >>
> >> On Sun, Oct 23, 2022 at 8:52 PM PengHui Li <peng...@apache.org> wrote:
> >>>
> >>> I support the motivation.
> >>>
> >>> When I read the document of Prometheus
> >>> I see "A label with an empty label value is considered equivalent to a
> >>> label that does not exist." [0]
> >>> Can we just keep the value of the topic and namespace label empty?
> >>> And here is a blog that introduced how to deal with the empty label [1]
> >>>
> >>> If this can work. We don't need to add new indicator names.
> >>> Otherwise, we might need "pulsar_tenant_*" ,"pulsar_namespace_*",
> >>> "pulsar_cluster_*" in the future.
> >>>
> >>> [0] https://prometheus.io/docs/concepts/data_model/
> >>> [1]
> >>>
> >> https://medium.com/pareture/query-results-where-label-is-not-present-in-prometheus-e1176320575d
> >>>
> >>> Thanks,
> >>> Penghui
> >>>
> >>> On Sun, Oct 23, 2022 at 6:14 PM Asaf Mesika <asaf.mes...@gmail.com>
> >> wrote:
> >>>
> >>>> One thing to note: You mentioned a PIP number but I'm not sure this is
> >> the
> >>>> right process you follow here for that.
> >>>>
> >>>>
> >>>> On Sun, Oct 23, 2022 at 1:12 PM Asaf Mesika <asaf.mes...@gmail.com>
> >> wrote:
> >>>>
> >>>>> The suggestion makes sense to me as well. I've also reviewed your PR.
> >>>>>
> >>>>> On Sun, Oct 23, 2022 at 8:43 AM Haiting Jiang <
> >> jianghait...@gmail.com>
> >>>>> wrote:
> >>>>>
> >>>>>> +1
> >>>>>> Makes sense to me.
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Haiting
> >>>>>>
> >>>>>> On Sat, Oct 22, 2022 at 3:59 AM yang yijun <yyiju...@gmail.com>
> >> wrote:
> >>>>>>>
> >>>>>>> Hi,I have a suggestion to improve the broker.
> >>>>>>>
> >>>>>>> For detailed improvement instructions, please refer to issue:
> >>>>>>> https://github.com/apache/pulsar/issues/18056
> >>>>>>>
> >>>>>>> For detailed source code change, please refer to PR:
> >>>>>>> https://github.com/apache/pulsar/pull/18116
> >>>>>>
> >>>>>
> >>>>
> >>
>
