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

Collection API #3

Merged
merged 28 commits into from
Feb 12, 2024
Merged

Collection API #3

merged 28 commits into from
Feb 12, 2024

Conversation

cmelone
Copy link
Collaborator

@cmelone cmelone commented Dec 25, 2023

This PR implements a webhook for the status change of Gitlab jobs. Build jobs are selected for processing, where a series of Prometheus queries are issued and data about the build and resource utilization are made available.

Adds: POST endpoint under /collect.

Priorities:

  • Minimize the amount of requests needed to collect the necessary data. At the moment, for builds: 1 request to Gitlab + 5 to Prometheus. For VMs: 2 requests to Prometheus.
  • Handling the potential for duplicates and multiple/erroneous requests for collection. Protections are implemented for race conditions.
  • Composability: API should be readable and potentially reusable for other purposes
  • Speed: while this application is written with async functionality, there is still a requirement to respond to the webhook in under 10 seconds.

All main functionality is implemented and I have removed the draft status from the PR. There are just a couple more things to do:

Also, I realize that this PR is quite big, so I'm happy to break it up into smaller pieces.

@cmelone cmelone self-assigned this Dec 25, 2023
gantry/utils/collect.py Outdated Show resolved Hide resolved
gantry/utils/collect.py Outdated Show resolved Hide resolved
@spack spack deleted a comment from girishbarca Jan 16, 2024
@cmelone cmelone changed the title Add build metadata collection collection webhook Jan 17, 2024
@cmelone cmelone added the feature New feature or request label Jan 18, 2024
@cmelone cmelone requested a review from alecbcs January 18, 2024 08:33
@cmelone cmelone marked this pull request as ready for review January 18, 2024 08:33
Copy link
Member

@alecbcs alecbcs left a comment

Choose a reason for hiding this comment

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

Super nice job! This PR looks great. Just a couple points of feedback.

I also might suggest creating the sqlite db in the root of the repo and actually putting db/ under gantry/ instead of leaving it as its own top level component. This might fit a bit better depending on how we structure the components. I'd probably go for something like the following, but feel free to push back if you don't find that as easy to work with.

gantry/
  db/
     get.py
     insert.py
  clients/
    prometheus.py
    gitlab.py
  models/
    jobs.py
    vms.py
  routes/
    collection.py
  __main__.py

.envrc Outdated Show resolved Hide resolved
gantry/collection.py Outdated Show resolved Hide resolved
gantry/collection.py Outdated Show resolved Hide resolved
gantry/collection.py Outdated Show resolved Hide resolved
gantry/collection.py Outdated Show resolved Hide resolved
gantry/models/build.py Outdated Show resolved Hide resolved
gantry/models/build.py Outdated Show resolved Hide resolved
gantry/util/gitlab.py Outdated Show resolved Hide resolved
gantry/util/misc.py Outdated Show resolved Hide resolved
gantry/views.py Outdated Show resolved Hide resolved
@cmelone
Copy link
Collaborator Author

cmelone commented Jan 24, 2024

I also might suggest creating the sqlite db in the root of the repo and actually putting db/ under gantry/ instead of leaving it as its own top level component.

I like this! Should I stick the .sql schema file in there as well?

@cmelone
Copy link
Collaborator Author

cmelone commented Jan 26, 2024

@alecbcs 11d05fc addresses your comments regarding the Prometheus client

lmk it that works

@cmelone
Copy link
Collaborator Author

cmelone commented Jan 26, 2024

Going to tackle testing in another PR to avoid adding too much more to this. marking this one as ready

@cmelone cmelone requested a review from alecbcs January 26, 2024 02:24
@cmelone cmelone mentioned this pull request Feb 5, 2024
2 tasks
alecbcs
alecbcs previously approved these changes Feb 5, 2024
Copy link
Member

@alecbcs alecbcs left a comment

Choose a reason for hiding this comment

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

Big fan of the rewrite! Just a few small nitpicks neither of which are real blockers if you prefer keeping as is. Otherwise looks good to me!

gantry/clients/prometheus/prometheus.py Outdated Show resolved Hide resolved
gantry/clients/__init__.py Outdated Show resolved Hide resolved
@alecbcs alecbcs changed the title collection webhook Collection API Feb 5, 2024
Copy link
Member

@alecbcs alecbcs 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 to me. @cmelone thank you for all of your work on this!

@alecbcs alecbcs merged commit 89353a1 into develop Feb 12, 2024
2 of 4 checks passed
@alecbcs alecbcs deleted the add/collection-func branch February 12, 2024 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants