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.

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

[FEATURE] - Add JobStatus to RecordJobTiming in Monitor Interface #775

Open
FalcoSuessgott opened this issue Sep 12, 2024 · 1 comment · May be fixed by #780
Open

[FEATURE] - Add JobStatus to RecordJobTiming in Monitor Interface #775

FalcoSuessgott opened this issue Sep 12, 2024 · 1 comment · May be fixed by #780
Labels
enhancement New feature or request

Comments

@FalcoSuessgott
Copy link

FalcoSuessgott commented Sep 12, 2024 •
edited
Loading

Morning fellas, really loving working with gocron, it allows me to quickly schedule repeating tasks! :)

Im currently building a Prometheus Exporter and using the Monitor interface to measure task durations and add the results to a Prometheus Histogram. I would love to be able to also add whether a task was successful or not as a label to the metric.

I was wondering whether its possible to add the JobStatus to the RecordJobTiming function, similar as IncrementJob already has it in the Monitor interface.

I believe, we could pass the JobStatus, based on the error result when running e.callJobWithRecover(j) in:

gocron/executor.go

Lines 399 to 408 in c84d8f7

err := e.callJobWithRecover(j)
e.recordJobTiming(startTime, time.Now(), j)
if err != nil {
_ = callJobFuncWithParams(j.afterJobRunsWithError, j.id, j.name, err)
e.incrementJobCounter(j, Fail)
} else {
_ = callJobFuncWithParams(j.afterJobRuns, j.id, j.name)
e.incrementJobCounter(j, Success)
}
}

Im aware, that this is probably a breaking change, but just wanted to get your thoughts on that one.
@FalcoSuessgott FalcoSuessgott added the enhancement New feature or request label Sep 12, 2024
@JohnRoesler
Copy link
Contributor

@FalcoSuessgott I was thinking you could add another method to the interface RecordJobTimingWithStatus, but that's breaking as well. I'm good with adding it, just trying to think through the best way to roll it out as far as releases

@FalcoSuessgott FalcoSuessgott linked a pull request Sep 19, 2024 that will close this issue
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(monitor): introduce MonitorStatus FalcoSuessgott/gocron
2 participants
@FalcoSuessgott @JohnRoesler