Is there potentially value in looking towards other options? 🦀 perhaps?

I believe so. I was thinking about using Rust, but it turned out I was a bit too lazy to learn a new language. Having said that, I would love to be part of an effort to implement this in Rust if that becomes the direction.

We can make this atomic easily by just using the distributed semaphore abstraction. That being said, I just wanted to make sure we are aware of this potential issue.

That makes sense @carlad-gl! I have updated the MR description with the goal and ask here, plus, marked the MR as draft to avoid accidental merge.

@pedropombeiro, @carlad-gl - Yes. I suggest that we eventually replace the current Gitlab::Json.safe_parse, because it actually parses the document twice, which IMHO, is not the best way to utilize the resources.

That being said, I would like to get feedback about whether it is worth the effort to maintain a Ruby extension. I believe it does because an order of magnitude performance improvement for a non-micro benchmark is huge, plus, we have already been maintaining and using a similar extension for the past 3 years.

Mehmet Emin INAC (de017ef4) at 17 Mar 13:12

@srushik - That's great to have a single worker to run this logic, but this service is also being called from the ingestion logic, right?

That's a good point. I would appreciate getting reviews for the Gem source code. Also, we can move it to the gitlab-org group just like oj-introspect.

@ghavenga, @ryaanwells - Can we please create an issue under the relevant epic to update scan profiles to run on tracked contexts?

Missing the redundant transaction sounds critical. Otherwise, good review.

This service can fail if it runs concurrently. Are we aware of this?

Thanks @zmartins, @ifrenkel! There is a conflict in the Gemfile. Would you mind resolving it?

Hi @gitlab-org/maintainers/rails-backend 👋 What are your thoughts about this change?

Mhmmm. I see that deprecation_toolkit supports adding exceptions based on stacktrace;

DeprecationToolkit::Configuration.allowed_deprecations = [
  ->(message, stack) { message ~= 'Foo' && stack.first.label == 'method_triggering_deprecation' }
]

Now I wonder if there is any benefit to having this signature-based approach instead 🤔

@stanhu - Do you mean using the deprecation_toolkit gem on its own, or using it to build this "stack trace signature" matching?

The idea behind this implementation is that when we deprecate a method, we raise an exception on dev and test environments whenever that method gets executed, but the main benefit of this implementation is that it will be possible to add exclusions based on the full application layer stacktrace.

For example, imagine we deprecate the id attribute of the User model. We will be able to add the following exception if it's already being used in the application layer logic.

["Project#add_member", "Project#is_member?", "User#id"]

You may ask, "Why don't we just add Project#is_member? as an exception?", but if we do so, people can keep building on top of the Project#is_member? method, which would keep adding more into the technical dept.

@kerrizor - No worries. I was genuinely curious to know(just to take as constructive feedback) if there was a broken pattern in the implementation that made you assume it was an automation glitch. The development history is here, but I wanted to have a fresh start after struggling a lot with the idea and implementation. I agree that having small, meaningful commits could help with this.

If have the information it could be helpful to surface in the deprecation warning?

@theoretick - Mhmmmm. The issue URL is printed out in the exception message, but maybe printing the owner group can be beneficial as well 🤔 I think I can programmatically get it from the bounded_contexts.yml. WDYT?

prompted, I assume

@kerrizor - I'm really curious to know; what makes you assume this?

Since it's fairly divergent from the main app, did you consider releasing this as an internal gem?

I think this makes sense. Creating a new gem usually involves more changes, as such a gem would require configuration options, but it still sounds like a good idea.

WDYT about adding logging of non-authorized call sites for production?

This can be a follow-up because I would like to introduce throttling/sampling rate etc., while implementing logging on production.

This is a case where I think it would be beneficial to prioritize adding docs.

💯

Good question. I think if I move all this logic to the gems/ directory, we can get rid of these magic comments.

Oups. I should have probably mentioned; this is coming from ActiveSupport core extensions.

Module.method(:deprecate)
=> #<Method: #<Class:Module>(Module)#deprecate(*method_names, deprecator:, **options) ***/lib/ruby/gems/3.3.0/gems/activesupport-7.2.3/lib/active_support/core_ext/module/deprecation.rb:17>