Skip to content

Common: Update check_expired_dids to use PrometheusPusher, updated SLQA2.0 guidelines #143

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Common: Update check_expired_dids to use PrometheusPusher, updated SLQA2.0 guidelines #143

wants to merge 1 commit into from

Conversation

@voetberg
Copy link
Contributor

@voetberg voetberg commented Aug 2, 2024

I'm redoing all of PR #132 with the updated sqla guidelines

@voetberg voetberg requested a review from dchristidis as a code owner August 2, 2024 20:46
@voetberg voetberg added the Common label Aug 2, 2024
dchristidis
dchristidis requested changes Aug 5, 2024
View reviewed changes
@voetberg voetberg force-pushed the 129-update-expired-dids branch from 4042f91 to c5a6bb7 Compare August 5, 2024 15:42
dchristidis
dchristidis requested changes Aug 6, 2024
View reviewed changes
@voetberg voetberg force-pushed the 129-update-expired-dids branch from 9483cf3 to 60ec26a Compare August 6, 2024 14:03
dchristidis
dchristidis requested changes Aug 6, 2024
View reviewed changes
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.

Please also address these:

check_expired_dids:43:60: W291 trailing whitespace
check_expired_dids:49:1: W293 blank line contains whitespace

Then I would either merge into a single commit or split based on objective (from what I can see: Python 2 compatibility, header, SQLAlchemy, Prometheus, except statement). Just make sure to prefix all commit subjects with Common: .

@voetberg voetberg force-pushed the 129-update-expired-dids branch from 60ec26a to 96bbc6c Compare August 6, 2024 14:25
@voetberg voetberg requested a review from dchristidis August 6, 2024 14:26
dchristidis
dchristidis requested changes Aug 6, 2024
View reviewed changes
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.

The content is perfect. Well done.

But I’m afraid I must pester you one last time about the commit message. It’s not considered a good practise to have overly-long subjects. Keep it short and elaborate in the body.

@voetberg voetberg force-pushed the 129-update-expired-dids branch from 96bbc6c to f6188e3 Compare August 6, 2024 15:13
@voetberg voetberg requested a review from dchristidis August 6, 2024 15:48
dchristidis
dchristidis previously approved these changes Aug 7, 2024
View reviewed changes
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.

Looks good, thanks for bearing with me.

Following the contribution guide, I need approval from @ericvaandering before I can merge.

@voetberg
Copy link
Contributor Author

voetberg commented Aug 7, 2024

Looks good, thanks for bearing with me.

Following the contribution guide, I need approval from @/ericvaandering before I can merge.

Eric is on vacation until next week so we'll have to wait. I already talked with him about this anyways so I assume it'll just be a formality.

dchristidis
dchristidis reviewed Aug 7, 2024
View reviewed changes
common/check_expired_dids Outdated

with PrometheusPusher() as manager:
(manager.gauge(
"expired_dids.total",
Copy link
Contributor

Choose a reason for hiding this comment

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

Requires further review.

Copy link
Contributor

@ericvaandering ericvaandering Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What is the "requires further review?"

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as the other two PRs: (1) it silently changes the metric name to (2) one that is arguably inferior.

@dchristidis dchristidis dismissed their stale review August 7, 2024 15:44

Uncommunicated metric rename

@ericvaandering
Copy link
Contributor

Looks good, thanks for bearing with me.
Following the contribution guide, I need approval from @/ericvaandering before I can merge.

Eric is on vacation until next week so we'll have to wait. I already talked with him about this anyways so I assume it'll just be a formality.

I assume the content-wise this is the exact same as #132 except for possibly some different SQLAlchemy grammar?

@voetberg
Copy link
Contributor Author

Looks good, thanks for bearing with me.
Following the contribution guide, I need approval from @/ericvaandering before I can merge.

Eric is on vacation until next week so we'll have to wait. I already talked with him about this anyways so I assume it'll just be a formality.

I assume the content-wise this is the exact same as #132 except for possibly some different SQLAlchemy grammar?

Yeah it's completely identical besides for the sql2.0 and some minor formatting/spacing/indenting

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

I approve. Can be merged.

@voetberg voetberg changed the title Modernize expired dids to use prom pusher Common: Update check_expired_dids to use PrometheusPusher, updated SLQA2.0 guidelines Aug 28, 2025
@voetberg voetberg force-pushed the 129-update-expired-dids branch from f6188e3 to 1db325d Compare August 28, 2025 20:24
@voetberg
Copy link
Contributor Author

@dchristidis Reverted the name change and added a probe_metrics gauge. Should be fine.

@dchristidis
Copy link
Contributor

Does this run for you? On my side it fails with an exception.

ValueError: Duplicated timeseries in CollectorRegistry: {rucio_probes_undertaker_expired_dids}

@ericvaandering ericvaandering mentioned this pull request Aug 29, 2025
Open
@voetberg
Common: Rewrite check_expired_dids.
0384ff0 
* Update to sqla2.0
* push with PrometheusPusher
* sort imports
* add header
* change except statement to except Exception
@voetberg voetberg force-pushed the 129-update-expired-dids branch from 1db325d to 0384ff0 Compare August 29, 2025 14:49
@voetberg
Copy link
Contributor Author

Does this run for you? On my side it fails with an exception.

ValueError: Duplicated timeseries in CollectorRegistry: {rucio_probes_undertaker_expired_dids}

Right you are. I missed testing it again after renaming the gauge. Corrected now.

@dchristidis
Copy link
Contributor

But then, I’m afraid, we come to the problem that PrometheusPusher doesn’t work for ATLAS yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dchristidis dchristidis dchristidis left review comments

@ericvaandering ericvaandering ericvaandering approved these changes

Assignees

@voetberg voetberg

Labels

Common

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@voetberg @ericvaandering @dchristidis