search

artprima / prometheus-metrics-bundle

Symfony 5/6 Prometheus Metrics Bundle
MIT License
129 stars 29 forks source link

AppMetrics missing status codes #6

Closed ns3777k closed 4 years ago

ns3777k commented 4 years ago

Hey! I've a couple of questions:

  1. Is it possible to turn off the bundled AppMetrics without traversing services and removing it? Like an options or something? I couldn't find one.
  2. Any reason why status codes are missing? I see 2xx, 4xx, etc. But a common case is to differentiate 404 from 403 (in grafana graphs for example). Should I submit a PR or add them somehow in my project?
denisvmedia commented 4 years ago
  1. No, there is no way to turn it off. But yes, it would be nice to add a feature to turn it off. But I'd prefer to keep it on by default, so that the bundle provides some metrics right after the installation. Do you have any suggestion on how to make it pluggable without traversing the services? You can actually, override the service itself by your own class, but I'm not sure if this is an acceptable solution.
  2. There is no particular reason, except that I didn't want to have a metric per status code (yes, distinguishing between 4xx codes could be important, but it's not that relevant for the other code types). I actually consider AppMetrics as an example. And as such, if we find a convenient way how to allow turning it off, this would let one create his own metrics instead of the default ones.
ns3777k commented 4 years ago

@denisvmedia Well, how about registering AppMetrics just like every custom metrics generator instead of doing this in bundle's services.xml?

Artprima\PrometheusMetricsBundle\Metrics\AppMetrics:
        tags:
            - { name: prometheus_metrics_bundle.metrics_generator }

Adding this in flex recipe will let it be on automatically(or follow README if flex is missing). Hence it'll be on for everyone by default and can be removed if necessary.

ns3777k commented 4 years ago

@denisvmedia what do you think ? :-)

denisvmedia commented 4 years ago

I'll get back to this issue in a couple of days, I need to think on it, and I didn't have a chance to devote enough time to this issue. Stay tuned ;)

ns3777k commented 4 years ago

ok :-) thx

denisvmedia commented 4 years ago

Ok, so... Let's divide it to two steps. First of all, I agree to remove the service by default. Also, I agree to include that in readme. This would be the first step. As soon as we done it, we can release a new version. After that, we can make a recipe that would get back the defaults. Why would we do that split into two steps? Simply because I think it is only possible to submit the recipes only after you have a version released, otherwise they will decline a pull request.

The only problem, however, is that I don't have time to do the stuff (I can make a pull request for the recipe, but that's it). Would you be eager to provide a pull request to the prometheus metrics bundle repo according to the agreed conditions? I would then release both of your changes so that we could speed up the process.

denisvmedia commented 4 years ago

cc @ns3777k

ns3777k commented 4 years ago

Hey! Actually I found out that this bundle has more disadvantages for me than I thought, so I wrote another one for my project. Things I needed:

  1. Is metricsGenerator is a really need feature? Most of the time you use only one to record request and response metrics.
  2. No information on how to create metrics not related to request / response
  3. Injecting CollectorRegistry into services requires to also inject namespace

I created only 1 listener to collect metrics from request and response and made a decorator service to wrap CollectorRegistry so I always have a namespace injected.

denisvmedia commented 4 years ago

Is metricsGenerator is a really need feature? Most of the time you use only one to record request and response metrics.

Well, no, record and request metrics are not the only ones that are required. At least in a generic case.