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

feat: Don't track metrics when request is made by a bot #217

Open
Bertg opened this issue Nov 27, 2024 · 6 comments
Open

feat: Don't track metrics when request is made by a bot #217

Bertg opened this issue Nov 27, 2024 · 6 comments
Assignees
Labels
enhancement New feature or request maybe later Something we may choose to do at a later date but not now

Comments

@Bertg
Copy link

Bertg commented Nov 27, 2024

Describe the feature request

It should be possible to disable metric collection at runtime, based on runtime data. This could be to ignore data collected from demo account, or feature checks triggered by bots.

Background

We monitor our website with betterstack. We are quite happy with it.
However, we have some feature flags that are validated when loading these monitored pages. This causes the metric data collected by unleash to be completely useless.

Solution suggestions

The current is_enabled?(feature, context = nil, default_value_param = false, &fallback_blk) api is starting to become hard to modify, as it is using positional arguments.

An idealised usage would look like this:

unleash_with_context = build_a_context_object # this is more ruby-ish as there is a good chance we want to re-use this in a web request (eg in rails)
# We could also explicetally set to disable the tracking on this context object, as the user agent would be unchanging and thus no need to re-evalute on each flag check
unleash_with_context.is_enabled?("flag_name", default: bool | proc(context, flag), track: bool | proc(context, flag)
@Bertg Bertg added the enhancement New feature or request label Nov 27, 2024
@sighphyre
Copy link
Member

I'm open to the idea. Probably have to let a few more colleagues weigh in first though.

This is probably as simple as just not calling the count functions in is_enabled? and get_variant. We happily accept PRs if you want to take a stab and don't break compatibility

@chriswk chriswk moved this from New to Todo in Issues and PRs Nov 28, 2024
@sighphyre
Copy link
Member

Okay, had a quick chat with colleagues. Sounds like there's some hesitation around changing the public API in this way, so maybe hold off on the PR. I think long term we'd like to be able to put useful filters on the metrics in UI and short term, one of the suggestions that came up was maybe not calling Unleash if you detect a bot.

I'm going to tag this as maybe later for now, if there's enough noise around this we'll take another closer look

@sighphyre sighphyre added the maybe later Something we may choose to do at a later date but not now label Nov 28, 2024
@Bertg
Copy link
Author

Bertg commented Nov 28, 2024

one of the suggestions that came up was maybe not calling Unleash if you detect a bot

Well, we do use unleash to put our site in maintenance mode... so that's not really a viable solution :p

I think long term we'd like to be able to put useful filters on the metrics in UI

That could also work, but that would mean we now need to send extra data along to the metrics endpoint, so you could filter on bots? That really looks like an over engineered solution.

I think I'll make a PR with a new class, similar to client, but that focuses a bit more on web/rails specific use cases. I can always publish it as a dedicated gem as well, if it would not be added to the gem.

@rarruda
Copy link
Collaborator

rarruda commented Nov 28, 2024

I also second being careful around changing the signature of the most important public method that we expose.

It is important also to be cohesive across SDKs.

But a separate method unleash_with_context.check_if_enabled_metrics_maybe? could an option maybe? IE: maybe a better name for the method?

If all else fails such a method can always be monkey patched.

@Bertg
Copy link
Author

Bertg commented Nov 30, 2024

So I created a POC of what I think could work here: https://github.com/Bertg/unleash-client-ruby/blob/request_client_poc/lib/unleash/request_client.rb

I can imagine it being used like this in a rails application:

class ApplicationController
  def unleash
    @unleash ||= Unleash::RequestClient.new(user_id: current_user&.id)
  end
end

def OtherController < ApplicationController
  def some_action
    unleash.enabled?('my_feature')
  end
end

def PublicController < ApplicationController
  before_action :disable_unleash_metrics_for_bots

  def disable_unleash_metrics_for_bots
    unleash.disable_metrics! if Botdetection.bot?(request)
  end
end

Maybe this strictly speaking doesn't belong in this gem, and there could be room for a unleash-rails gem, or something of that nature.

What do you think?

@Bertg
Copy link
Author

Bertg commented Dec 9, 2024

@rarruda @sighphyre Would you be open to receive a full PR with these changes? If not I'll instead work on building a companion gem instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request maybe later Something we may choose to do at a later date but not now
Projects
Status: Todo
Development

No branches or pull requests

3 participants