+1, Great work.
Haiting On Thu, Nov 24, 2022 at 6:25 PM Jiuming Tao <jm...@streamnative.io.invalid> wrote: > > Hi Haiting, > > I’ll update the PIP as we discussed: > > 1. `pulsar_broker_rest_endpoint_failed` > Labels: > a. path: The HTTP request path > b. method: The HTTP request method > c. code: The HTTP response code > This metric only records HTTP requests which response code >= 400 > > 2. `pulsar_broker_rest_endpoint_latency` > Labels: > a. path: The HTTP request path > b. method: The HTTP request method > c. code: The HTTP response code > This metric records all the HTTP requests, including failed requests. > > > Thanks, > Tao Jiuming > > Haiting Jiang <jianghait...@gmail.com> 于2022年11月24日周四 16:35写道： > > > Hi Jiuming, > > > > > I’m not sure that if observe failed requests latency is meaningful. > > > > 1. Normally for failed requests, the latency sometimes will help in > > debugging. > > 2. Add httpcode label to latency, we can analyze the latency between > > different successful results too, like 200 and 307. > > > > As for `pulsar_broker_rest_endpoint_failed`, after a second thought. I > > think it would be useful for operators to set up system alarms. So +1 > > on this. > > > > Thanks, > > Haiting > > > > On Thu, Nov 24, 2022 at 12:52 AM Jiuming Tao > > <jm...@streamnative.io.invalid> wrote: > > > > > > Hi Haiting, > > > > > > I’m not sure that if observe failed requests latency is meaningful, so > > that I was added `pulsar_broker_rest_endpoint_failed` to record these > > requests which is failed. > > > > > > Thanks, > > > Tao Jiuming > > > > > > > 2022年11月23日 下午12:55，Haiting Jiang <jianghait...@gmail.com> 写道： > > > > > > > > Hi Jiuming, > > > > > > > > I am not sure why we need a new metric for failed requests. > > > > Can we just put the `httpcode` tag also in the > > > > `pulsar_broker_rest_endpoint_latency_ms`? > > > > So that user can also see the latency of failed requests (like timeout > > > > requests). > > > > And it can easily cover the case of failed metrics. > > > > > > > > Thanks, > > > > Haiting > > > > > > > > On Wed, Nov 23, 2022 at 12:00 PM Jiuming Tao > > > > <jm...@streamnative.io.invalid> wrote: > > > >> > > > >> Hi Penghui, > > > >> > > > >>> We'd better add an option to enable or disable the endpoint-level > > metrics. > > > >>> And keep it as disabled as default. > > > >> > > > >> Agreed, I also considered if we need a configuration to > > enable/disable the feature, seems add an option is better > > > >> > > > >>> So that we can > > > >>> get the request path from the handle method > > > >>> `public void handle(String path, Request baseRequest, > > HttpServletRequest > > > >>> request, HttpServletResponse response)` > > > >> > > > >> I’ve tried this, but the `path` is the request path like > > `/user/111/query`, and we cannot get the path configured by @Path by using > > `Handler`. > > > >> Jetty is a Servlet container, and we are using Jersey to provide Rest > > API services, I think use Jersey’s mechanism is better > > > >> > > > >> Thanks, > > > >> Tao Jiuming > > > >> > > > >> > > > >> > > > >>> 2022年11月23日 上午9:37，PengHui Li <peng...@apache.org> 写道： > > > >>> > > > >>> Hi, Jiuming > > > >>> > > > >>> Thanks for starting the proposal. > > > >>> > > > >>> We'd better add an option to enable or disable the endpoint-level > > metrics. > > > >>> And keep it as disabled as default. > > > >>> > > > >>> I noticed the existing jetty metrics are based on > > > >>> `org.eclipse.jetty.server.handler.StatisticsHandler`. > > > >>> Can we just have a new StatisticsHandler? e.g. > > EndpointStatisticsHandler. > > > >>> So that we can > > > >>> get the request path from the handle method > > > >>> `public void handle(String path, Request baseRequest, > > HttpServletRequest > > > >>> request, HttpServletResponse response)` > > > >>> > > > >>> Thanks, > > > >>> Penghui > > > >>> > > > >>> On Tue, Nov 22, 2022 at 5:20 PM Jiuming Tao > > <jm...@streamnative.io.invalid> > > > >>> wrote: > > > >>> > > > >>>> Hi Haiting, > > > >>>> > > > >>>> I'm sorry I didn't explain FAILED, the FAILED means the HTTP > > response code > > > >>>>> = 400, and I’ll update the PIP later. > > > >>>> > > > >>>> Thanks, > > > >>>> Tao Jiuming > > > >>>> > > > >>>>> 2022年11月22日 下午5:15，Haiting Jiang <jianghait...@gmail.com> 写道： > > > >>>>> > > > >>>>> Hi Jiuming, > > > >>>>> > > > >>>>> Overall, this PIP makes sense to me. > > > >>>>> About the metric "pulsar_broker_rest_endpoint_failed", please > > provide > > > >>>>> a more clear definition of "fail". Are redirects like 403 included? > > > >>>>> > > > >>>>> Thanks, > > > >>>>> Haiting > > > >>>>> > > > >>>>> On Tue, Nov 22, 2022 at 3:17 AM Jiuming Tao > > > >>>>> <jm...@streamnative.io.invalid> wrote: > > > >>>>>> > > > >>>>>> Hi pulsar community, > > > >>>>>> > > > >>>>>> I’ve opened a PIP to discuss: PIP-223: Add metrics for all Rest > > > >>>> Endpoints > > > >>>>>> > > > >>>>>> The PIP link: https://github.com/apache/pulsar/issues/18560 < > > > >>>> https://github.com/apache/pulsar/issues/18560> > > > >>>>>> > > > >>>>>> Thanks, > > > >>>>>> Tao Jiuming > > > >>>> > > > >>>> > > > >> > > > > >
