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

[Metrics] Add original job definition name to pipeline metrics tags #441

Merged
merged 2 commits into from
Oct 18, 2024

Conversation

Emile-Filteau
Copy link
Member

When tracking pipeline metrics, we currently can only filter down by pipeline name, executor name and job id. There is currently no way of filtering them by job name.

I would even argue that we should maybe remove the jobId from the tags, because it has a very high cardinality.

I'm not sure i'm happy with adding this field right on the QueueItem. I wonder if we should have some sort of tags dictionary instead containing both the executor name and the job definition name?

Another avenue would be to leverage the job labels, but i feel like this might be mixing up some concepts.

@isra17
Copy link
Member

isra17 commented Oct 17, 2024

Another avenue would be to leverage the job labels, but i feel like this might be mixing up some concepts.

I think it could make sense no? Kubernetes does it for a few fields. You can namespace label key such as saturn/job-definition-name=some-job-name

@Emile-Filteau
Copy link
Member Author

My main concern is that we already have all the job definition labels on it.

You would suggest adding a special key to that labels dict that we know we can recover later?

@isra17
Copy link
Member

isra17 commented Oct 17, 2024

My main concern is that we already have all the job definition labels on it.

You would suggest adding a special key to that labels dict that we know we can recover later?

Could just simply add the label to the template here: https://github.com/Flared/saturn/blob/main/src/saturn_engine/worker_manager/config/declarative_job_definition.py#L34 ?

@Emile-Filteau Emile-Filteau force-pushed the efilteau/add-job-name-to-usage-metrics branch from dc96a9e to 43e0539 Compare October 17, 2024 19:04
@Emile-Filteau
Copy link
Member Author

Updated the PR, i changed the approach so that we now simply add the job definition name as a default label on the QueueItem. Since we already collect all the labels in saturn.pipeline.message we will now also have the job name.

I also added the labels to saturn.pipeline.usage which was only adding the pipeline and executor as tags.

Copy link
Member

@isra17 isra17 left a comment

Choose a reason for hiding this comment

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

Nice, one comment, but lgtm otherwise!

src/saturn_engine/worker_manager/config/declarative_job.py Outdated Show resolved Hide resolved
@Emile-Filteau Emile-Filteau merged commit 8c09df2 into main Oct 18, 2024
3 checks passed
@Emile-Filteau Emile-Filteau deleted the efilteau/add-job-name-to-usage-metrics branch October 18, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants