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

Add Meter for in-progress jobs #139

Merged
merged 5 commits into from
Nov 26, 2024
Merged

Add Meter for in-progress jobs #139

merged 5 commits into from
Nov 26, 2024

Conversation

ptoffy
Copy link
Member

@ptoffy ptoffy commented Nov 25, 2024

These changes are now available in 1.17.0

This adds a Meter metric to record the number of jobs currently being processed by a worker.
The gauge also allows calculation of not yet processed but enqueued jobs, since we can now do

notYetProcessedJobs = dispatchedJobsCount - (errorCompleted + successCompleted) - inProgressJobs

Timer(
label: "\(jobName).jobDurationTimer",
label: "\(jobName).duration.timer",
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if this can be considered a breaking change, but people who use this might have to switch

Copy link
Member

Choose a reason for hiding this comment

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

I think this would break integrations. SemVer wise it's technically fine but it could cause issues. We're not actually changing any information here right? Though the display unit was incorrect before right?

@maciejtrybilo you're using this correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the name back. The display unit is probably worth keeping as seconds is just not a great metric for job duration IMO

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would break integrations. SemVer wise it's technically fine but it could cause issues. We're not actually changing any information here right? Though the display unit was incorrect before right?

@maciejtrybilo you're using this correct?

I am, but it's no catastrophe to change the units. I'm good!

dimensions: [
("success", error == nil ? "true" : "false"),
("jobName", jobName),
],
preferredDisplayUnit: .seconds
preferredDisplayUnit: .milliseconds
Copy link
Member Author

Choose a reason for hiding this comment

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

As above

@ptoffy ptoffy changed the title Add Gauge for in-progress jobs Add Meter for in-progress jobs Nov 25, 2024
Copy link

codecov bot commented Nov 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.57%. Comparing base (c502c4a) to head (dadc6ac).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #139      +/-   ##
==========================================
+ Coverage   84.48%   84.57%   +0.08%     
==========================================
  Files          22       22              
  Lines         709      765      +56     
==========================================
+ Hits          599      647      +48     
- Misses        110      118       +8     
Files with missing lines Coverage Δ
Sources/Queues/QueueWorker.swift 97.24% <100.00%> (+0.58%) ⬆️

... and 8 files with indirect coverage changes

@ptoffy ptoffy added semver-patch Internal changes only semver-minor Contains new APIs and removed semver-patch Internal changes only labels Nov 26, 2024
@ptoffy ptoffy merged commit 7511f2d into main Nov 26, 2024
11 checks passed
@ptoffy ptoffy deleted the metrics branch November 26, 2024 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor Contains new APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants