[BUG] DeprecationLogger - Unbounded memory usage

[rajiv-kv]

Describe the bug

DeprecationLogger dedupes messages that has been already logged based on value of X-Opaque-Id provided in request.
Keys set is maintained in DeprecatedMessage. The entries in set are unbounded and are appended based on the user provided header value X-Opaque-Id. In case the user ends up passing unique Opaque-Id per request it can result in OOM on node.

Related component

Other

To Reproduce

1. Create a system index in cluster say system-test-index-1
2. curl -H "X-Opaque-Id: <timestamp-in-milliis>" "https://<endpoint>/_all/_alias/test-alias"
3. Repeat step 2 for 10K times and you will notice increased heap memory usage

Expected behavior

DeprecatdMessage#keys should be memory bounded and not result in out-of-memory.

Additional Details

Plugins
Please list all plugins currently enabled.

Screenshots
If applicable, add screenshots to help explain your problem.

Host/Environment (please complete the following information):

• OS: [e.g. iOS]
• Version [e.g. 22]

Additional context
Add any other context about the problem here.

[dblock]

@rajiv-kv Thanks for raising this, definitely an issue. Want to contribute a fix? Maybe a circuit breaker?

[rajiv-kv]

@rajiv-kv Thanks for raising this, definitely an issue. Want to contribute a fix? Maybe a circuit breaker?

Sure @dblock - can explore.

[dblock]

This was introduced in #1660, @VachaShah @andrross we all missed this. I think the map should both have an expiration for its items and be capped in size (in bytes) + be adjustable.

[andrross]

I think the map should both have an expiration for its items and be capped in size (in bytes) + be adjustable.

@dblock I think a fixed-sized LRU cache with no expiration would probably be fine? If you happen to fill up the cache then you start evicting the least recently accessed items, potentially leading to duplicated log messages for those keys. I'd definitely like to avoid the complexity of making it adjustable. We probably don't need to overthink this because the goal is to de-dupe most messages to avoid spamming the log.

[shwetathareja]

@andrross Yes, the first thing is that it should be bounded so that it doesn't keep on growing. But, I would like to understand the logic behind opaque-id to be appended, if all requests have unique id it doesn't help at all.

[rajiv-kv]

Is there a way for user to see the X-Opaque-Id in logs ? I am trying to understand if there is any downside if we track the messages at per feature (Key) rather than per request (KeyWithXOpaqueId).

[dblock]

I think a fixed-sized LRU cache with no expiration would probably be fine?

Totally. +1 on not overthinking it. I think the only concern may be with a high TPS, but in that case this whole logger may be pointless. Maybe a setting to turn it off? I'm overthinking it again ...

Is there a way for user to see the X-Opaque-Id in logs ? I am trying to understand if there is any downside if we track the messages at per feature (Key) rather than per request (KeyWithXOpaqueId).

Yes, the intent was to help the caller who has access to the logs to find where the deprecated requests are coming from, not for the operator to know that a deprecated API is called by someone. It shows this:

[2021-07-01T05:18:51,291][DEPRECATION][o.o.d.c.m.IndexNameExpressionResolver] [0e77307b90eb] this request accesses system indices: [.opendistro_security], but in a future major version, direct access to system indices will be prevented by default

[shwetathareja]

@dblock @andrross Thanks for updating the documentation and stating the usage of X-Opaque-Id header. But this may not be enough, we need to audit all the official clients like OpenSearch-hadoop to ensure it is used correctly. In case, a user gets into trouble simply because the opensearch clients are mis-using the header, that's not good experience.

[dblock]

There are many references to X-Opaque-Id, https://github.com/search?q=org%3Aopensearch-project%20X-Opaque-Id&type=code, anyone care to go through them to see if there's problems and open issues?