Skip to content

Conversation

@mnbbrown
Copy link
Contributor

@mnbbrown mnbbrown commented Nov 7, 2022

This is a bit of a speculative PR.. not sure of it's impact - it could be abused and I think the implementation would definitely need some checks on payload size, etc.

That said, it could be very useful for trace context propogation and prevent hacks we see here like https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/instrumentation/que/lib/opentelemetry/instrumentation/que/patches/que_job.rb#L26-L39

If you think it's a good idea I'll finish the implementation, and write some tests.

@ZimbiX
Copy link
Member

ZimbiX commented Nov 7, 2022

I think I had seen that. Sounds like a good idea to me

@dtcristo
Copy link
Contributor

I would like this change.

In our application we've got a custom "wrapperless" ActiveJob Que adapter (instead of the default "wrapped" adapter that stores everything in args) and exposing the data column in this way would be very helpful to easily allow storing ActiveJob metadata there.


tags = job_options[:tags] || []
data = { tags: tags }.merge(job_options[:data] || {})
# TODO: some protection about overall data size
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need protection for this? If people want to store arbitrarily large data here it should be allowed right? Since important indexes like que_poll_idx and que_jobs_args_gin_idx don't include this column, large data shouldn't have too much of an impact on performance.

end

tags = job_options[:tags] || []
data = { tags: tags }.merge(job_options[:data] || {})
Copy link
Contributor

Choose a reason for hiding this comment

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

With this implementation you'll always get a tags key and empty array in the situation where there are no tags.

Perhaps something like this instead?

tags = job_options[:tags] ? { tags: job_options[:tags] } : {}
data = tags.merge(job_options[:data] || {})

@hlascelles
Copy link
Contributor

One thing we would like to put in here is "when was a job initially scheduled" as that info is lost when a job retries.

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.

4 participants