Support more tags in prometheus metrics

[PhilippHeuer]

Implementation for issue #23 to add more tags to the prometheus metrics endpoint.

This adds the following 4 configuration options (tag_*) to the prometheus config:

"extra_config": {
      "github_com/philippheuer/krakend-opencensus": {
        "sample_rate": 100,
        "reporting_period": 1,
        "exporters": {
          "prometheus": {
            "port": 9091,
            "namespace": "krakend",
            "tag_host": true,
            "tag_path": true,
            "tag_method": true,
            "tag_statuscode": true
          }
        }
      }
    },

It will iterate over all client / server metrics and add the tags if they were not present yet, if the value is set to true - does nothing if its not set or false, so its not a breaking change for users of the metrics right now.

I'm not sure yet if those tags also affect influxdb metrics, if they do the config options might need to be moved into the toplevel of the opencensus config.

I'm open for feedback as i'm not too familiar with opencensus or krakend yet ;)

Are the configuration options fine like that or do we want this configurable on a per metric base? or just a format option which has a few valid values, ie. normal / extended?

[kpacha]

great, but I have some concerns about the memory consumption

[PhilippHeuer]

I've now added both the server and client part.
The client pattern gets reported as /services/{{.The}}/{{.World}} and the server pattern as /services/:the/:world, directly as i get them from the krakend config. We could still turn this into a common format.

When updating kraken-ce to this, it will be required to change line 50 in backend_factory.go to return opencensus.HTTPRequestExecutor(clientFactory, cfg) to pass the backend config to the http executor.

[oskoi]

@PhilippHeuer @kpacha, Hi! What's the status of this PR? The PR looks interesting to me. If I could help something, let me know.

[PhilippHeuer]

The state is as described above, the only open point is:

The client pattern gets reported as /services/{{.The}}/{{.World}} and the server pattern as /services/:the/:world, directly as i get them from the krakend config.

I still think we should decide on a common format which will be used to report both server and endpoint metrics. Personally i would prefer /services/:the/:world.
I'm not sure if there is a best-practice on how to report parameters in metrics - at least i don't know of any. If no one disaggress i would change it to report the format as /services/:the/:world.

I already build a custom kraken ce with the modified opencensus module and it was working fine in my tests - but i didn't do any performance tests so i don't know what kind of performance impact this has. (as i also didn't notice the transport wrapper that was recreated on each request)

[oskoi]

As for me pattern option will be enough. It's the only option that cannot create the high cardinality at runtime.

lastparam is the more dubious option. I.e: I have the server pattern as /services/:entity/:userId and use lastparam option. After some updates I change the pattern as /services/:entity/:entityId/:userId where entityId is a dynamic param and forget to update the option. This creates a risk of performance problems.

I think it would be better to use the format that is used in the configuration, i.e.: /services/{the}/{world} because different routers use a different format: gin - /:id, mux - /{id}.

@PhilippHeuer @kpacha what do you think?

[PhilippHeuer]

@oskoi i personally would still prefer to provide the parameters in some kind of defined format regardless of the used router implementation.

If the tags change because the used router implementation is changed in the future (gin -> mux) then it would break all dashboards / reporting / whatever you do with those metrics since the same endpoint would suddently have a different path tag.

[oskoi]

@PhilippHeuer yes, I also meant this. I suggested using the agnostic format that is used by krakend in a config file.

[PhilippHeuer]

@oskoi thats a good idea, i didnt think about the format used in the config file ...

[oskoi]

@kpacha @PhilippHeuer friendly ping :)

what's the status of PR?

[kpacha]

@oskoi , the branch has conflicts and the build is failing...

[grahamsutton]

My team and I at work could really use this feature as well. What can I do to help get this moving?

[alombarte]

@PhilippHeuer you invested a lot of time in this issue, but unfortunately, it couldn't be merged because of the conflicts. Are you still interested in the contribution?

Thanks

[PhilippHeuer]

@alombarte yes, i will resolve the open conflicts.

I would test everything again first to make sure its still working and look if i can remember any open tasks/todos.

[PhilippHeuer]

I resolved the conflicts.

While reviewing this again i resolved the following issues:

• Client Metrics are reported as: /hello/:hello/:world / The BE Metrics are reported as: /services/{{.Hello}}/{{.World}} - so i added some code to normalize this to match the configuration file format /{hello}/{world}
• The lastparam aggregation wasn't working correctly for client metrics
• Paths are not case-sensitive, so if a user uppercased a single letter this would result in a new metric for same same endpoint/service - resolved by lowercasing the entire path value.

I also created a pr to kraken-ce as draft for a one-line change that is required after this pr gets accepted.
-> krakend/krakend-ce#278

Questions:

• Is there a documentation for configuration options of the opencensus module somewhere, where we need to add the new new parameters + possible values?

I think this is good for a new review.

[rottenbytes]

Upvote for this PR 👍 Thanks for this amazing work @PhilippHeuer

[kpacha]

I'd create a new function HTTPRequestExecutorFromConfig so we don't break the interfaces and signatures exposed by the package

internally, the HTTPRequestExecutor function could just call the new one with a nil configuration

[PhilippHeuer]

The code for backend path aggregration needs the config though, otherwise it can't access the configured pattern and only the requested url.
So to prevent users of the old method from running into the cardinality issue, i think we could return a empty string as path if no config is available.

[kpacha]

@PhilippHeuer this looks pretty good to me. I'm merging it.

thanks for the contribution and for your patience with me and the review process!!!

[alombarte]

Hi @PhilippHeuer, let me thank you personally for having followed this PR for a year and a half. I promise it wasn't intentional, neither a test, but if it were you would have passed it with the highest score.

The functionality will be incorporated on the next KrakenD release 1.4 (or maybe 2.0). I want to also thank you on behalf of anonymous companies I cannot mention as this merge was really awaited.

Let me know if there is anything we can do for you, just ASK (albert AT krakend.io)

[github-actions]

This pull request was marked as resolved a long time ago and now has been automatically locked as there has not been any recent activity after it. You can still open a new issue and reference this link.