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

Common: rewrite check_transfer_queue_status #147

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

voetberg
Copy link
Contributor

@voetberg voetberg commented Aug 6, 2024

  • Rewrite base query with sqla2.0
  • Move CASE statements into python logic - This I am less confident I got it correct, I could not figure out exactly what the external_host is supposed to look like. The probe has been broken a long time in CMS int and prod so I'm missing some reference material there.
  • Sort Imports
  • Update header
  • Update exception statement
  • Change gauge to PrometheusPusher

@voetberg voetberg requested a review from dchristidis as a code owner August 6, 2024 15:23
@voetberg voetberg added the Common label Aug 6, 2024
common/check_transfer_queues_status Outdated Show resolved Hide resolved
common/check_transfer_queues_status Outdated Show resolved Hide resolved
common/check_transfer_queues_status Outdated Show resolved Hide resolved
common/check_transfer_queues_status Outdated Show resolved Hide resolved
common/check_transfer_queues_status Outdated Show resolved Hide resolved
@voetberg voetberg force-pushed the 129-transfer-queue-status branch from 4ad7968 to 1d4c3ba Compare August 7, 2024 13:40
common/check_transfer_queues_status Outdated Show resolved Hide resolved
common/check_transfer_queues_status Show resolved Hide resolved
common/check_transfer_queues_status Outdated Show resolved Hide resolved
* Rewrite base query with sqla2.0
* Move CASE statements into python logic
* Sort Imports
* Update header
* Update exception statement
* Change gauge to PrometheusPusher
@voetberg voetberg force-pushed the 129-transfer-queue-status branch from 1d4c3ba to 884bbcd Compare August 7, 2024 14:09
Copy link
Contributor

@dchristidis dchristidis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done. I’m happy on my end.

Copy link
Contributor

@dchristidis dchristidis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I‘m afraid I have to withdraw my approval.

There is a significant change that was not mentioned at all in the commit: the metric name has been changed from conveyor_queues_requests to rucio_probes_transfer. Also state now contains the queues.requests. prefix, which doesn’t look right to me.

Similarly, the other two PRs I approved will have to be revisited.

@voetberg
Copy link
Contributor Author

There is a significant change that was not mentioned at all in the commit: the metric name has been changed from conveyor_queues_requests to rucio_probes_transfer.

I think the Prometheus pusher automatically appends rucio_probes, but otherwise I can swap the transfer part of the label back.

Also state now contains the queues.requests. prefix, which doesn’t look right to me.

That was from the original query (unless I missed a post-processing step in the original? That case statement just makes my head spin so it's a possibility)

@voetberg voetberg self-assigned this Aug 20, 2024
@ericvaandering
Copy link
Contributor

Correct. "rucio_probes" (or "rucio.probes" for Graphite) is added by the pusher

PROBES_PREFIX = 'rucio.probes'

I'm going to approve this.

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

Successfully merging this pull request may close these issues.

3 participants