can we make this consistent with Greenwave? Usually WaiverDB and Greenwave have the same paths since are "twins" :) Greenwave exposes the metrics at /metrics

Why is this import here and not at the begging of the file?

What "messaging_sent_failed_counter" stands for? If it is the number of messages not sent I wouldn't call it "messaging sent", since it wasn't actually sent.

Why do you have this counter if you already have the "messaging_sent_passed_counter"?

You are increasing "messaging_sent_counter" before this, if it will hit the "continue" it won't publish a message and it wouldn't make sense to increase "messaging_sent_counter"

why changing this logic?

I'm not sure we want to actually raise an exception if an error occurs, if we don't raise an error it will just continue with the next item of the loop. It would be ok to log it.

where is this used?

I can do it. I was thinking forward on the /monitor/ endpoint as a "home" for other things like healthchecks etc., as proposed in "Factory 2.0 Monitoring docs". If we do it the other way round, I'd prepare a PR for changing it in Greenwave.

What "messaging_sent_failed_counter" stands for? If it is the number of messages not sent I wouldn't call it "messaging sent", since it wasn't actually sent.

Why do you have this counter if you already have the "messaging_sent_passed_counter"?

You are increasing "messaging_sent_counter" before this, if it will hit the "continue" it won't publish a message and it wouldn't make sense to increase "messaging_sent_counter"

Sorry, the naming was already fixed in the monitor module, but not here.. -> Fixed now.

where is this used?

This is a generic set of metrics, not used on waiverdb.

why changing this logic?

There shouldn't be changes in logic. Just changes to de-indent:
if...else -> if not

As I added one more level, it started to be unreadable.

I'm not sure we want to actually raise an exception if an error occurs, if we don't raise an error it will just continue with the next item of the loop. It would be ok to log it.

There shouldn't be any difference. If the sender(s) raised an exception before, this just re-raises it... The only difference is that it gets caught and counters are incremented...

It is fine if we use this approach, but yeah, I would like it to be the same for both WaiverDB and Greenwave. We just have to make sure no one is already using /metrics in Greenwave (I don't think so).

where is this used?

This is a generic set of metrics, not used on waiverdb.

So is it useful to keep it if we don't use it?

where is this used?
This is a generic set of metrics, not used on waiverdb.

So is it useful to keep it if we don't use it?

It'd more difficult to maintain various versions of the monitor module for each service if the generic part would be different. Most of other services publish/consume messages. But WaiverDB just publishes (from what I know). I am open to ideas of keeping it slim but also maintainable.

It is fine if we use this approach, but yeah, I would like it to be the same for both WaiverDB and Greenwave. We just have to make sure no one is already using /metrics in Greenwave (I don't think so).

I think that internal deployment would need to make slight modifications to annotations in the templates, and that should be all.

It'd more difficult to maintain various versions of the monitor module for each service if the generic part would be different. Most of other services publish/consume messages. But WaiverDB just publishes (from what I know). I am open to ideas of keeping it slim but also maintainable.

This is causing also failings on our CIs... https://jenkins-waiverdb-test.cloud.paas.upshift.redhat.com/job/waiverdb-test/job/waiverdb-test-waiverdb-dev/162/console
I understand your point, but wiaverdb is still a independent service and I'm not really happy about having stuff that we don't need in the code.

Optional: You could just use raise instead of needing the e variable

Why not just use the path /monitor/metrics that points directly to this view? I don't see any benefit from making path a variable to parse. It just adds complexity in my opinion.

@mprahl @gnaponie I shrank the code to fit just the waiverdb needs. The etalon can be found in a separate repo, see monitor.py's header.

The CI is still failing... these errors:
08:43:09 [Invoke Pylint] E: 11, 0: Unable to import 'prometheus_client' (import-error)
08:43:09 [Invoke Pylint] W: 76, 0: Unused argument 'kw' (unused-argument)
08:43:09 [Invoke Pylint] W: 80,31: Unused argument 'conn' (unused-argument)
08:43:09 [Invoke Pylint] W: 80,37: Unused argument 'branch' (unused-argument)
08:43:09 [Invoke Pylint] W: 84,29: Unused argument 'exception_context' (unused-argument)
08:43:09 [Invoke Pylint] W: 88,25: Unused argument 'conn' (unused-argument)
08:43:09 [Invoke Pylint] W: 76, 4: Unused variable 'receive_dbapi_error' (unused-variable)
08:43:09 [Invoke Pylint] W: 80, 4: Unused variable 'receive_engine_connect' (unused-variable)
08:43:09 [Invoke Pylint] W: 84, 4: Unused variable 'receive_handle_error' (unused-variable)
08:43:09 [Invoke Pylint] W: 88, 4: Unused variable 'receive_rollback' (unused-variable)
08:43:09 [Invoke Pylint] W: 11, 0: Unused Histogram imported from prometheus_client (unused-import)

See PR#275 which block this PR on the CI. Then we can continue here...

You should add the keyword argument content_type=prometheus_client.CONTENT_TYPE_LATEST since this will set the content type of plain text with the metrics output version that python-prometheus_client is using.

You should add the keyword argument content_type=prometheus_client.CONTENT_TYPE_LATEST since this will set the content type of plain text with the metrics output version that python-prometheus_client is using.

Thanks for pointing this out. I have to do it F2.0-projects-wide. :)

The CI failed again, it looks like an error in Jenkins itself. Could you re-trigger the job?

@gnaponie Please for final review here. All comments have been addressed.

Why this is not imported at the beginning of the life?

From PEP8 doc:
Imports should be grouped in the following order:

Standard library imports.
Related third party imports.
Local application/library specific imports.
You should put a blank line between each group of imports.

Why is that before "from flask import current_app"?
Is there some issue with circular imports?

Just to avoid cyclic imports. But the noqa seems to be some relic from previous revisions.

Can we add the log of the error here?

Logging is not related to this PR, but I'll add some _log.exception there.

Beside the minor comments it looks good +1

@gnaponie Build 181 Finished: SUCCESS for 62e7388.

looks good to me +1

Commit 5265247c fixes this pull-request