WIP: refactor(metrics): migrate from prometheus client to opentelemetry

[alexppg]

With OT, it will be easier to switch backends when we want to.

It would solve the metrics part of this issue. #181.

[alexppg]

It mostly works, I'm only having one issue with the Histogram. Waiting until somebody answers, since the documentation is pretty poor. open-telemetry/opentelemetry-python#1229

[Agalin]

It seems that OpenTelemetry has yet to define a way to run under Gunicorn / multiprocess environment (see https://github.com/open-telemetry/opentelemetry-python/issues/365) so I'd argue that for now metrics should support both direct Prometheus integration (with #179 fixes) and OpenTelemetry for different use cases (maybe simply as different services).

[alexppg]

I agree. I've been playing with OT for a couple of days and I believe that we should treat them, for now, as beta features, for both metrics and traces. It's still on very early days, it has some limitations.

[coveralls]

Pull Request Test Coverage Report for Build 1065

• 9 of 82 (10.98%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-3.8%) to 95.381%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pyms/flask/app/create_app.py	9	20	45.0%
pyms/flask/services/opentelemetry.py	0	62	0.0%

Totals
Change from base Build 1055:	-3.8%
Covered Lines:	1838
Relevant Lines:	1927

---

💛 - Coveralls

[alexppg]

The open-telemetry/opentelemetry-python#1229 issue is fixed, but I've found the next one. open-telemetry/opentelemetry-python#1255

[alexppg]

Ok, so the issue is that Histograms still don't exist on OT with Prometheus backend. I guess I'll just leave them out until there's some support, so I can finish this MR and begin the others, which might be more useful.

[alexppg]

@avara1986 I'm having some doubts defining the opentelemetry service.
For example, in this section. There may be tens of instrumentations: aiohttp, flask, sqlite3, jinja, mysql, ... There's already more than 15. This means that in this section, there will grow a lot.
My question is, should this conditionals be directly on the service? Some of them have to be here, but only a couple or so. This would make this section lighter in the future, but this seems to not be the way the rest of the services are doing this, so maybe for consistency this shouldn't.

[alexppg]

@avara1986 Do you think it make sense to add this to the PR? They won't be useful, since the tracing and logging are out of scope, but it may be useful to have it already designed.

[avara1986]

with the new method of serviceDriver def init_action(self, microservice_instance): could be more easy to read the other services. But we need to check other way to check the services that add if service, if service, if service... ;)

[Agalin]

Open telemetry is a kind of a meta service. As it may instrument other services (there is an issue for redis integration already afair) it seems that configuring integration should actually be a responsibility of the service that provides instrumented module. Maybe in similar style to ‘init_action’ there should be ‘init_telemetry’ that would be called by opentelemetry service? Or is it too tightly coupled?
There is just no way to generically initialise integrations. Don’t know why did they do it like that. For example sentry sdk just gets a list of integrations to enable - most (if not all) of those work well without any params.

[alexppg]

What you're proposing that for every service there's a telemetry section? It may make sense, but not all possible instrumentation of OT will be available as service.
Also, maybe you don't want to use the service that PyMS provides (for example requests) but you do want OT for requests.
Are you talking about this?

[Agalin]

I've checked code for some connectors that would be useful for services (not only documentation this time) and while some of example instrumentations take params it seems to really be just an optional tracer. So it's possible to configure those by simply iterating over a list after all. There aren't that many instrumentations that need custom setup and would be useful here - Celery, Flask, WSGI, maybe a few more that may work here due to not being async-related. But requests also uses optional callback so advanced config may be useful here. Also not sure if support for multiple tracers is important?
But still find per-service telemetry section appealing - instead of something this:

opentelemetry:
      integrations:
      - redis
      - requests

you end with something like this:

redis:
      telemetry: true
requests:
      telemetry: true

About you don't want to use the service but you do want OT I'd argue that if you're not using the provided service then instrumenting this part is on you - either in init_libs or new app-level init_telemetry - as it may be necessary to enable instrumentation before lib usage. Alternatively it can be done like this:

redis:
      telemetry: true
      service_enabled: false
requests:
      telemetry: true

But there is one major advantage when using OT instrumentation list in config: those simple instrumentations may be enabled completely without a service - or even without any additional support in pyms. But then something like this would need to be suported:
opentelemetry:
integrations:
- opentelemetry.instrumentation.redis.RedisInstrumentor

[alexppg]

Do you have an opinion about optional imports? With this service there will be a lot of imports, at least one per instrumentation.
Should I be doing the same done on traces here? https://github.com/python-microservices/pyms/blob/master/pyms/flask/services/tracer.py#L5

[Agalin]

Why not import using https://github.com/pallets/werkzeug/blob/master/src/werkzeug/utils.py#L778 - it's already a flask dependency.