Skip to content

Improve slug generation for crons #2138

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

Closed
cleptric opened this issue Oct 13, 2023 · 5 comments · Fixed by #2168
Closed

Improve slug generation for crons #2138

cleptric opened this issue Oct 13, 2023 · 5 comments · Fixed by #2168
Assignees
Labels

Comments

@cleptric
Copy link
Member

I have a question regarding the Ruby SDK. In the documentation, it states

# slug defaults to the job class name
sentry_monitor_check_ins slug: 'custom_slug'

Currently it appears that the slug parameter is always required, since Ruby class names can contain punctuation (::) and will have capital letters.

e.g.
SuperFancy::PrivateGroups::SpecialOrganizations::ScheduleOutreachWorker

In the code, I would suggest that they use self.class.name.parameterize as the default, which will downcase the class name and replace the double colons with a dash.

[1] pry(main)> e=SuperFancy::PrivateGroups::SpecialOrganizations::ScheduleOutreachWorker.new
=> #<SuperFancy::PrivateGroups::SpecialOrganizations::ScheduleOutreachWorker:0x00007f13c7a366f0>
[2] pry(main)> e.class.name.parameterize
=> "superfancy-specialgroups-specialorganizations-scheduleoutreachworker"
[3] pry(main)> e.class.name.parameterize.size
=> 68

This example demonstrates the other issue that we're running into, which is one of length. Since the Dashboard UI constrains the value to 50 characters (this not mentioned in the documentation), it appears the slug parameter will still have to be used.

Are these assumptions correct?

Originally posted by @nimylian in getsentry/sentry#42283 (comment)

@sl0thentr0py sl0thentr0py changed the title I have a question regarding the Ruby SDK. In the [documentation](https://docs.sentry.io/platforms/ruby/crons/#job-monitoring), it states Better slug generation for crons Oct 16, 2023
@sl0thentr0py sl0thentr0py changed the title Better slug generation for crons Improve slug generation for crons Oct 16, 2023
@swanson
Copy link

swanson commented Nov 3, 2023

One concern I have is that it appears slugs can be duplicated across environments -- could/should the gem prefix the Rails environment to the slug?

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 Nov 3, 2023
@natikgadzhi
Copy link
Contributor

This example demonstrates the other issue that we're running into, which is one of length. Since the Dashboard UI constrains the value to 50 characters (this not mentioned in the documentation), it appears the slug parameter will still have to be used.

Would it be better to try and omit pieces of the default slug if it runs out of 50 characters, or would it be more beneficial to omit the middle part of the longer slugs in the Dashboard UI? I would assume Dashboard will have to deal with many things that are too long to display, eventually?

@sl0thentr0py
Copy link
Member

thanks for the feedback everyone, I'll think about a good way to do this when I have some time to fix this.
Will keep all the above in mind!

@sl0thentr0py
Copy link
Member

ok so I will parameterize and truncate from the beginning, I think that's more useful since the job name stays at the end

@sl0thentr0py
Copy link
Member

sl0thentr0py commented Nov 13, 2023

@swanson the environment is already sent as part of the check-in in the event payload, so they will already be categorized correctly.

image

y-yagi added a commit to y-yagi/sentry-ruby that referenced this issue Nov 29, 2023
`sidekiq-scheduler` allows to use a class name as a schedule name directly as follows.

```
Namspeced::CancelAbandonedOrders:
  cron: '0 */5 * * * *'
```

In this case, a slug is shown as `namspecedcancelabandonedorders` on Sentry.
I think this isn't good for readability.
This PR converts `::` to `-` as well as the cron default behavior of Crons mixin.

Related with: getsentry#2138.
y-yagi added a commit to y-yagi/sentry-ruby that referenced this issue Nov 29, 2023
`sidekiq-scheduler` allows to use a class name as a schedule name directly as follows.

```
Namspeced::CancelAbandonedOrders:
  cron: '0 */5 * * * *'
```

In this case, a slug is shown as `namspecedcancelabandonedorders` on Sentry.
I think this isn't good for readability.
This PR converts `::` to `-` as well as the cron default behavior of Crons mixin.

Related with: getsentry#2138.
y-yagi added a commit to y-yagi/sentry-ruby that referenced this issue Nov 29, 2023
`sidekiq-scheduler` allows to use a class name as a schedule name directly as follows.

```
Namspeced::CancelAbandonedOrders:
  cron: '0 */5 * * * *'
```

In this case, a slug is shown as `namspecedcancelabandonedorders` on Sentry.
I think this isn't good for readability.
This PR converts `::` to `-` as well as the cron default behavior of Crons mixin.

Related with: getsentry#2138.
y-yagi added a commit to y-yagi/sentry-ruby that referenced this issue Nov 29, 2023
`sidekiq-scheduler` allows to use a class name as a schedule name directly as follows.

```
Namspeced::CancelAbandonedOrders:
  cron: '0 */5 * * * *'
```

In this case, a slug is shown as `namspecedcancelabandonedorders` on Sentry.
I think this isn't good for readability.
This PR converts `::` to `-` as well as the cron default behavior of Crons mixin.

Related with: getsentry#2138.
st0012 pushed a commit that referenced this issue Nov 29, 2023
`sidekiq-scheduler` allows to use a class name as a schedule name directly as follows.

```
Namspeced::CancelAbandonedOrders:
  cron: '0 */5 * * * *'
```

In this case, a slug is shown as `namspecedcancelabandonedorders` on Sentry.
I think this isn't good for readability.
This PR converts `::` to `-` as well as the cron default behavior of Crons mixin.

Related with: #2138.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants