Maybe Logger metadata should affect grouping

[ruslandoga]

Right now logged errors seem to be grouped by message only.

iex> Logger.error("error from logs", sentry: %{extra: %{response: Bamboo.ApiError.build_api_error("info 1")}})
iex> Logger.error("error from logs", sentry: %{extra: %{response: Bamboo.ApiError.build_api_error("info 2")}})
# 16:06:06.217 [warning] Event dropped due to being a duplicate of a previously-captured event.
:ok

Would it make sense to consider a hash of :sentry metadata as well?

[whatyouhide]

Mmmm what do we do for errors, do you know? I haven't looked and won't have time til the end of the day.

[solnic]

@whatyouhide this is what we do:

  # Used to compare events for deduplication. See "Sentry.Dedupe".
  @doc false
  @spec hash(t()) :: non_neg_integer()
  def hash(%__MODULE__{} = event) do
    :erlang.phash2([
      event.exception,
      event.message,
      event.level,
      event.fingerprint
    ])
  end

Adding metadata to this sounds like a good idea to me.

[whatyouhide]

I’m not sure because metadata might contain fleeting data that gets updated on every call (imagine a timestamp) but that shouldn't affect deduplication. What do other SDKs do?

[solnic]

I’m not sure because metadata might contain fleeting data that gets updated on every call (imagine a timestamp) but that shouldn't affect deduplication. What do other SDKs do?

I'm not sure - @sl0thentr0py do you happen to know?

[whatyouhide]

@solnic you could probably grep through source code of at least the Python and Ruby SDKs because I remember finding this in there pretty easily.

[solnic]

@whatyouhide I did look for it in ruby and couldn't find it 😅 I'll give it another shot hah

[ruslandoga]

Maybe:

• allow disabling deduplication for loggers (Sentry sinks can do rate-limiting per grouping if needed)
• deduplicate at least on three (or so) the available fields (can be though of degrees of freedom in this scenario), e.g. if for a Logger.error only two of these are present, level (which is always error? so it's not really a degree of freedom) and message, we can pick additional optional field, e.g. metadata from the call, and we can use a probabilistic view of the metadata fields
• replace exact hash matching with SimHash/MinHash and some similarity measure
• we just make this issue a note in documentation and explain how to set a custom fingerprint on Logger.error calls

[sl0thentr0py]

@solnic

• ruby doesn't have any deduplication on the SDK
• python only does deduplication of the actual exception object instance, so doesn't touch logger messages

You can either

• not run the dedupe logic for logger messages at all
• add the sentry metadata to the hash

either way is fine with me. In general, we have comprehensive server side grouping, so these SDK side dedupes are mainly for not sending the exact same payload twice.

[whatyouhide]

either way is fine with me. In general, we have comprehensive server side grouping, so these SDK side dedupes are mainly for not sending the exact same payload twice.

Since that's the case, I think it makes more sense to include the meta in the deduplication then, or do the deduplication at a later stage (so that logger messages just become Sentry.Events and we do deduplication there).

[ruslandoga]

I think it makes more sense to include the meta in the deduplication

I'd be happy to open a PR with this approach if this issue is not taken yet! :)

or do the deduplication at a later stage (so that logger messages just become Sentry.Events and we do deduplication there).

Isn't that what's already happening? As per #837 (comment)

  def hash(%__MODULE__{} = event) do
    # for a log event it's something like
    :erlang.phash2([
      _exception = nil,
      _message = "error from logs",
      _level = "error",
      _fingerprint = nil
    ])
  end

[whatyouhide]

Even for events the deduplication might be buggy here, because user/tags/extra should probably be factored in. So a PR is absolutely more than welcome! 💟