The committers listed above are authorized under a signed CLA.

• ✅ login: matschaffer / name: Mat Schaffer (2099f3f, d429dfb)

Codecov Report

Merging #3034 (a56beed) into main (5f9ef51) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #3034   +/-   ##
=======================================
  Coverage   92.64%   92.64%           
=======================================
  Files         187      187           
  Lines        6169     6169           
  Branches     1303     1303           
=======================================
  Hits         5715     5715           
  Misses        454      454           

@matschaffer I don't think thats an issue to re-use it however you'll need to add a entry in experimetanl/CHANGELOG.md and sign the CLA so we can accept the contribution !

Thanks @vmarchaud ! Should be all set on those points.

Can’t you use the request handler that’s available on PrometheusExporter?

I am using exporter. getMetricsRequestHandler(req, res)

@weyert I tried that but didn't see a good way to map from the KibanaResponseFactory API (https://github.com/elastic/kibana/blob/fa89c459ac71a9c911a5a2cc486dd3eff120d484/src/core/server/http/router/response.ts#L117) to ServerResponse.

Similarly I wasn't sure how to cleanly pass in the unused IncomingMessage without an ugly as any.

Can you share the code you ended up using?

Oh sorry, I am not well versed in Kibana but I had a quick look and at first sight it looked
like you could use: https://github.com/elastic/kibana/blob/fa89c459ac71a9c911a5a2cc486dd3eff120d484/src/core/server/http/router/router.ts#L338

I am using it with Express so having less of a problem but I am doing something like:

this_app.use((req, res) => {
    If (this._exporter) {
           this.exporter.getMetricsRequestHandler(req, res)
    } else {
          res.status(404).json({ message: ‘Not OK })
    }
})

Thanks! I'll give another crack at it today.

It feels like a bit of extra overhead to have a server-off PrometheusExporter as compared to a reusable PrometheusSerializer. But maybe the smaller public API surface area is worth that trade off.

Yeah, that’s why I am using the handler and the prevent the server form starting to avoid a second server instance just for th metrics endpoint. That was my main reason for introducing the method in the past

@vmarchaud @weyert so while getMetricsRequestHandler is handy for express cases, I don't think I can use it in a Kibana API. There are services upstream of me that need to ensure appropriate headers on responses as well.

To use that API I'd have to feed it an instantiated ServerResponse then read back & parse just the body to pass into KibanaResponseFactory.

As such I'd like to propose we merge this API. That will enable me to use the PrometheusSerializer directly from my own kibana-compatible version of the PrometheusExporter.

As shown on elastic/kibana#128755 (comment), the approach is functional but it'd be great not to have to fork PrometheusSerializer into the kibana repo to do so.

Thanks in advance!

No worries, I just tried to unblock you . Happy with the PR myself. I have approved it as an outsider ;)

@legendecas easy enough. I'll put it on my list for tomorrow. Thanks for the feedback 🧡

@legendecas does the project have any typical strategy for testing private methods? I was thinking I'll try updating the tests to work via the serialize method.

Some other ideas are:

• Remove the direct testing: this would leave PrometheusSerializer with only 2 its
• Use protected and test a public subclass: doesn't really address concern of public API size
• Use serializer['...']() to route around the type check failures: seems a little "dirty"

Initial attempts at testing via serialize are looking good so far.

@matschaffer generally options 1 and 3 are both fine. We already have practices on 3, i.e. testing with the bracket accessing, as it still preserves type information and TypeScript can properly check the type conformance.

Thanks for the quick reply! Testing via serialize worked out well I think. Just needed to introduce a helper to skip the comments it adds. Have a look and let me know what you think!

Thanks @dyladan !