You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pulsar.apache.org by Jiuming Tao <jm...@streamnative.io.INVALID> on 2022/11/21 19:17:46 UTC
[DISCUSS] PIP-223: Add metrics for all Rest Endpoints
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
Re: [DISCUSS] PIP-223: Add metrics for all Rest Endpoints
Posted by Jiuming Tao <jm...@streamnative.io.INVALID>.
Yes, it makes sense, it is better to keep aligned with other settings.
Thanks,
Tao Jiuming
Michael Marshall <mm...@apache.org> 于2022年12月9日周五 13:41写道：
> Thanks for the proposal. I think this will be a valuable addition, and
> I wonder if it makes sense to add a similar proposal to optionally
> monitor the binary protocol in the same way.
>
> One minor question about naming. I see that we have the following
> boolean configurations with "metrics" in the name:
>
> private boolean replicationMetricsEnabled = true;
> private boolean authenticateMetricsEndpoint = false;
> private boolean exposeTopicLevelMetricsInPrometheus = true;
> private boolean metricsBufferResponse = false;
> private boolean exposeConsumerLevelMetricsInPrometheus = false;
> private boolean exposeProducerLevelMetricsInPrometheus = false;
> private boolean exposeManagedLedgerMetricsInPrometheus = true;
> private boolean exposeManagedCursorMetricsInPrometheus = false;
> private boolean exposeBundlesMetricsInPrometheus = false;
>
> I notice the trend as "expose.*MetricsInPrometheus". What do you think
> about changing "enablePerRestEndpointMetrics" to
> "exposePerRestEndpointMetricsInPrometheus"?
>
> Thanks,
> Michael
>
> On Thu, Nov 24, 2022 at 11:30 PM Haiting Jiang <ji...@gmail.com>
> wrote:
> >
> > +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 <ji...@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 <ji...@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 <pe...@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 <ji...@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
> > > > > >>>>
> > > > > >>>>
> > > > > >>
> > > > >
> > > >
>
Re: [DISCUSS] PIP-223: Add metrics for all Rest Endpoints
Posted by Jiuming Tao <jm...@streamnative.io.INVALID>.
A proposal for monitoring binary protocol is in planning now
Thanks,
Tao Jiuming
Michael Marshall <mm...@apache.org> 于2022年12月9日周五 13:41写道：
> Thanks for the proposal. I think this will be a valuable addition, and
> I wonder if it makes sense to add a similar proposal to optionally
> monitor the binary protocol in the same way.
>
> One minor question about naming. I see that we have the following
> boolean configurations with "metrics" in the name:
>
> private boolean replicationMetricsEnabled = true;
> private boolean authenticateMetricsEndpoint = false;
> private boolean exposeTopicLevelMetricsInPrometheus = true;
> private boolean metricsBufferResponse = false;
> private boolean exposeConsumerLevelMetricsInPrometheus = false;
> private boolean exposeProducerLevelMetricsInPrometheus = false;
> private boolean exposeManagedLedgerMetricsInPrometheus = true;
> private boolean exposeManagedCursorMetricsInPrometheus = false;
> private boolean exposeBundlesMetricsInPrometheus = false;
>
> I notice the trend as "expose.*MetricsInPrometheus". What do you think
> about changing "enablePerRestEndpointMetrics" to
> "exposePerRestEndpointMetricsInPrometheus"?
>
> Thanks,
> Michael
>
> On Thu, Nov 24, 2022 at 11:30 PM Haiting Jiang <ji...@gmail.com>
> wrote:
> >
> > +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 <ji...@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 <ji...@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 <pe...@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 <ji...@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
> > > > > >>>>
> > > > > >>>>
> > > > > >>
> > > > >
> > > >
>
Re: [DISCUSS] PIP-223: Add metrics for all Rest Endpoints
Posted by Michael Marshall <mm...@apache.org>.
Thanks for the proposal. I think this will be a valuable addition, and
I wonder if it makes sense to add a similar proposal to optionally
monitor the binary protocol in the same way.
One minor question about naming. I see that we have the following
boolean configurations with "metrics" in the name:
private boolean replicationMetricsEnabled = true;
private boolean authenticateMetricsEndpoint = false;
private boolean exposeTopicLevelMetricsInPrometheus = true;
private boolean metricsBufferResponse = false;
private boolean exposeConsumerLevelMetricsInPrometheus = false;
private boolean exposeProducerLevelMetricsInPrometheus = false;
private boolean exposeManagedLedgerMetricsInPrometheus = true;
private boolean exposeManagedCursorMetricsInPrometheus = false;
private boolean exposeBundlesMetricsInPrometheus = false;
I notice the trend as "expose.*MetricsInPrometheus". What do you think
about changing "enablePerRestEndpointMetrics" to
"exposePerRestEndpointMetricsInPrometheus"?
Thanks,
Michael
On Thu, Nov 24, 2022 at 11:30 PM Haiting Jiang <ji...@gmail.com> wrote:
>
> +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 <ji...@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 <ji...@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 <pe...@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 <ji...@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
> > > > >>>>
> > > > >>>>
> > > > >>
> > > >
> > >
Re: [DISCUSS] PIP-223: Add metrics for all Rest Endpoints
Posted by Haiting Jiang <ji...@gmail.com>.
+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 <ji...@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 <ji...@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 <pe...@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 <ji...@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
> > > >>>>
> > > >>>>
> > > >>
> > >
> >
Re: [DISCUSS] PIP-223: Add metrics for all Rest Endpoints
Posted by Jiuming Tao <jm...@streamnative.io.INVALID>.
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 <ji...@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 <ji...@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 <pe...@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 <ji...@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
> > >>>>
> > >>>>
> > >>
> >
>
Re: [DISCUSS] PIP-223: Add metrics for all Rest Endpoints
Posted by Haiting Jiang <ji...@gmail.com>.
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 <ji...@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 <pe...@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 <ji...@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
> >>>>
> >>>>
> >>
>
Re: [DISCUSS] PIP-223: Add metrics for all Rest Endpoints
Posted by Jiuming Tao <jm...@streamnative.io.INVALID>.
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 <ji...@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 <pe...@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 <ji...@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
>>>>
>>>>
>>
Re: [DISCUSS] PIP-223: Add metrics for all Rest Endpoints
Posted by Haiting Jiang <ji...@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 <pe...@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 <ji...@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
> >>
> >>
>
Re: [DISCUSS] PIP-223: Add metrics for all Rest Endpoints
Posted by Jiuming Tao <jm...@streamnative.io.INVALID>.
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 <pe...@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 <ji...@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
>>
>>
Re: [DISCUSS] PIP-223: Add metrics for all Rest Endpoints
Posted by PengHui Li <pe...@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 <ji...@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
>
>
Re: [DISCUSS] PIP-223: Add metrics for all Rest Endpoints
Posted by Jiuming Tao <jm...@streamnative.io.INVALID>.
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 <ji...@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
Re: [DISCUSS] PIP-223: Add metrics for all Rest Endpoints
Posted by Haiting Jiang <ji...@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