Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ConnexionPrometheusMetrics - mimetype #64

Closed
jo-gre opened this issue Jul 13, 2020 · 4 comments
Closed

ConnexionPrometheusMetrics - mimetype #64

jo-gre opened this issue Jul 13, 2020 · 4 comments

Comments

@jo-gre
Copy link

jo-gre commented Jul 13, 2020

Hey there,
sorry that I bother you again. The new Connexion release works mostly fine with my Code but I discovered a new problem.
Calling the get_response method from Connexion without mimetype as parameter leads to the warning:

FutureWarning: Implicit (flask) JSON serialization will change in the next major version. This is triggered because a response body is being serialized as JSON even though the mimetype is not a JSON type. This will be replaced by something that is mimetype-specific and may raise an error instead of silently converting everything to JSON. Please make sure to specify media/mime types in your specs.
  warnings.warn(

That also leads to the problem, that the response.json part is equal None and API response can only be found in the response.data part.

Setup:

Updated example from last time:

from pydantic.dataclasses import dataclass
from foo.main import metrics


@dataclass
class Info:
    foo: str


@metrics.histogram("foo", "bar", labels={"foo": lambda: "bar"})
def test() -> Info:
    return Info('Test version')
import connexion
from prometheus_flask_exporter import ConnexionPrometheusMetrics

app = connexion.App(__name__)
metrics = ConnexionPrometheusMetrics(app, export_defaults=None, excluded_paths=".")


if __name__ == '__main__':
    app.add_api('my_api.yaml')
    app.app.run(port=5000, use_reloader=False)

openapi: 3.0.0
info:
  version: 1.0.0
  title: Test

paths:
  /test:
    get:
      operationId: endpoint.test
      responses:
        '200':
          description: Test

Expected behaviour:

curl -v GET "http://127.0.0.1:5000/test" 
* Could not resolve host: GET
* Closing connection 0
curl: (6) Could not resolve host: GET
*   Trying 127.0.0.1:5000...
* Connected to 127.0.0.1 (127.0.0.1) port 5000 (#1)
> GET /test HTTP/1.1
> Host: 127.0.0.1:5000
> User-Agent: curl/7.71.1
> Accept: */*
> 
* Mark bundle as not supporting multiuse
* HTTP 1.0, assume close after body
< HTTP/1.0 200 OK
< Content-Type: application/json
< Content-Length: 28
< Server: Werkzeug/1.0.1 Python/3.8.3
< Date: Mon, 13 Jul 2020 09:34:18 GMT
< 
{
  "foo": "Test version"
}
* Closing connection 1

Actual behaviour:

curl -v GET "http://127.0.0.1:5000/test" 
* Could not resolve host: GET
* Closing connection 0
curl: (6) Could not resolve host: GET
*   Trying 127.0.0.1:5000...
* Connected to 127.0.0.1 (127.0.0.1) port 5000 (#1)
> GET /test HTTP/1.1
> Host: 127.0.0.1:5000
> User-Agent: curl/7.71.1
> Accept: */*
> 
* Mark bundle as not supporting multiuse
* HTTP 1.0, assume close after body
< HTTP/1.0 200 OK
< Content-Type: text/html; charset=utf-8
< Content-Length: 28
< Server: Werkzeug/1.0.1 Python/3.8.3
< Date: Mon, 13 Jul 2020 09:34:23 GMT
< 
{
  "foo": "Test version"
}
* Closing connection 1

The fact that the content-type is not application/json sadly breaks some API callers.

@rycus86
Copy link
Owner

rycus86 commented Jul 13, 2020

Hm, that's interesting, not sure how it's handled there implicitly. Thanks for reporting!
Would following the recommendation in the warning help here?

Please make sure to specify media/mime types in your specs.

@jo-gre
Copy link
Author

jo-gre commented Jul 13, 2020

This probably refers to the possibility of defining endpoints like this:

paths:
  /test:
    get:
      operationId: endpoint.test
      responses:
        '200':
          description: Test
          content:
            application/json:
              type: object
              properties:
                version:
                  type: string

But I tried this already this is not recognized by the function itself. I think this makes the API call the response function with:

mimetype: "application/json"

If the function is called with that it will work. I thought about an additional kwargs parameter like you added it before. But the problem there would be that maybe not every Endpoint has the same response type...

@rycus86
Copy link
Owner

rycus86 commented Jul 13, 2020

I think I saw the default mimetype set to json in the Connexion source code, maybe that's what's missing here. I'm starting to have some doubts whether I implemented the decorators the right way here (as they seem to process the response before the wrapping framework), so I might have a look at that later this week, time permitting.

Until then, the best I could offer is to switch back to plain PrometheusMetrics and pass in your own response_converter that applies to your specific use case.
I'll update this issue if I find out how to handle this properly.

Thanks again for reporting it, and for providing examples! 🙏

@rycus86
Copy link
Owner

rycus86 commented Jul 22, 2020

So, I couldn't solve this properly, but I made it closer to the Connexion defaults.
The ConnexionPrometheusMetrics constructor now accepts a default_mimetype parameter that defaults to application/json if not set, this is in line with the default mimetype in Connexion.

Unfortunately, the actual mimetype (based on the content types in the OpenAPI spec) are quite well hidden in Connexion and I couldn't find a way to look those up at runtime. :(
The workaround I added is that now you can decorate the view functions with an explicit content type override like this:

@metrics.content_type('text/plain')
@metrics.counter('test_plain', 'Counter for plain responses')
def plain() -> str:
    return 'Test plain response'

It's not very good, but it does work around the issue.

If I have time, I might see whether we could integrate the OpenAPI spec parsing from Connexion and perhaps we could pick the default content type from there, but I feel like it's never going to be 100%

What do you think @Echronix does this provide any usefulness for what you use Connexion for, for example?

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants