[BUG] Refactor DocStatusStats

[msfroh]

Describe the bug

While reviewing #18405, I ended up going down a rabbit hole of looking at existing indexing stats, where I stumbled across DocStatusStats, which violates so many rules of our stats:

1. Stats should have a clear separation between mutable accumulators (generally an implementation of the Metric interface) and immutable snapshots. For indexing stats, the mutable counters are in InternalIndexingStats and the immutable snapshot is supposed to be IndexingStats. DocStatusStats is a mutable set of counters arbitrarily crammed into IndexingStats. (Oh, also DocStatusStats uses AtomicLongs, when properly-implemented Metric instances use LongAdder, which would also be more appropriate here if not for all the other things wrong with it.)
2. DocStatusStats has a toXContent method that's reading from mutable state. The values returned are not guaranteed to be true for any particular instant, since they could be changing within the toXContent call.
3. Everything else in IndexingStats is tracking shard-level metrics. Then there's this weird DocStatusStats crammed in that gets computed on the coordinator node. Which only "works" because it's abusing OldShardStats, which is meant to hold statistics for shards that were previously held on this node, but closed.

The PR that introduced DocStatusStats should never have been merged. Since it was, we should go back and clean up this mess.

Related component

Indexing

To Reproduce

1. Read OpenSearch code
2. See this abomination
3. Claw out your eyes

Expected behavior

Consistent architecture for stats APIs.

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.

[harshavamsi]

To Reproduce

Read OpenSearch code
See this abomination
Claw out your eyes
😆

[ajleong623]

@msfroh I am working on a parallel issue in #18377 with pull request and currently have a pr out. I am glad I found this issue though because in my pr, I reused a lot of ideas from the pr you pointed out. I did not realize that the original pr violated a lot rules. To maintain the integrity of the codebase I would be happy to refactor both my implementation with SearchResponseStatusStats and the DocStatusStats one.

I think the biggest issue where the location of DocStatusStats is in IndexingStats and it is abusing the OldShardStats. From my understanding of what was mentioned. OldShardStats should not be updated because those shards are supposed to be closed. Would it be better if the DocStatusStats was moved into its own file?

However, I am also still thinking of a better way to update the same statistics outside of using the IndicesService. Let me know if you have any immediate ideas, but in the meantime, I will search through the codebase to look at how other stats are updated as I am not super familiar with everything.

[msfroh]

Hey @ajleong623 -- I'm glad you noticed this issue too!

Right now, the biggest problem with DocStatusStats is probably the fact that it misses the whole idea that threadsafe mutable counters collect stats, and immutable snapshots get returned. On top of that, because the existing IndexingStats are collected per-shard, but we wanted a "global" counter, instead of thinking "I should create a new node-level counter", DocStatusStats was crammed into the shard-level IndexingStats. Since there's no one shard associated with a _bulk request, it was just randomly shoved into OldShardStats.

Both for indexing and for search, it sounds like we're just looking for some good old-fashioned node-level counters, which should be tied to some singleton. I don't hate tying DocStatusStats to IndicesService as long as its not tied to (shard-level) IndexingStats. For example, SearchRequestStats is (IMO) a better model. That's a singleton created in the Node constructor and tied to IndicesService.

Where I think SearchRequestStats slightly falls apart is that the mutable SearchRequestStats gets passed to the immutable NodeIndicesStats here:

OpenSearch/server/src/main/java/org/opensearch/indices/IndicesService.java

Lines 803 to 805 in fe3f454

	return new NodeIndicesStats(commonStats, statsByShard(this, flags), searchRequestStats, statsLevel);
	} else {
	return new NodeIndicesStats(commonStats, statsByShard(this, flags), searchRequestStats);

.

IMO, SearchRequestStats should expose a method that returns an immutable snapshot at the current point in time. We have the classes for those immutable snapshots, but they're in SearchStats (as RequestStatsLongHolder and PhaseStatsLongHolder). Instead of IndicesService passing SearchRequestStats into NodeIndicesStats, it should get the immutable snapshot and pass that. (Right now, IndicesService is also the place where shard-level counters get turned into immutable snapshots, in the indexShardStats method.)

Phew... that's a long-winded explanation, but I hope it clears some things up and helps point you in the right direction.

[msfroh]

Oh -- also, more fundamentally, I think that counting underlying HTTP statuses (especially if you're going to only keep the first digit) is a ridiculous idea.

You need 3 counters: success, failure due to user error, and failure due to system error. Not everything has to be an HTTP status code.

[ajleong623]

@msfroh I agree that having the counters not tied to http statuses will be better for the users because having the 3 counters probably makes it more readable. For the other points, it seems like the 2 main concerns is keeping the counter out of the shard level classes since it is a global one and separating the immutable from the mutable stats and only taking from the immutable stats.

I will go back to the code to look for how the structure of immutable and mutable stats looks like as well as the shard vs global separation. Thank you for providing the example with SearchRequestStats as well as your in depth explanation, and I will get back to this when I push a solution.

[ajleong623]

@msfroh Ok, so I just pushed the new, refactored changes for the doc status and the search request status. I made sure that when the counters are shown to the user the names are shown rather than the underlying status.

Additionally, the counters are still in the indices section of the stats, but they are no longer in the shard-level index stats. Now, the LongAdder is used instead of AtomicLong. Instead of using the oldIndicesStats, a reference to the holder of the status counters is used in IndicesService. The holder of both the search status counter and doc status counter has a method getSnapshot which returns an immutable point in time copy of all of the counters, and this will be passed into NodeIndicesStats just like SearchRequestStats. StatusCounterStats which holds all the stats counters is initialized in IndicesService which is also a singleton as I believe there is only one IndicesService instance per node.

These changes are all available in the pull request linked above. There might be some tests that I have to sort out because of merge conflicts, but you could see the changes I made in the pr above.

Please let me know of any and all feedback you have on anything around this issue or if you have any questions or clarifications. I am still learning quite a bit from this project and in general. Also, sorry that it took some time to get back to this issue. I got a bit sidetracked on a couple of other issues on only really jumped into it the past couple of days. And thank you for your patience and your diligence in pointing these issues out.

[msfroh]

Thank you so much @ajleong623! The state of DocStatusStats was a mistake that shouldn't have gone in the way it did. Unfortunately, set a precedent for doing the same thing again.

Thank you for not just doing the new thing the right way, but also cleaning up the mistakes of the past.

[ajleong623]

@msfroh It is no problem. I am glad I could make a positive impact and work towards maintaining the integrity of this codebase.

Of course, I also want to make sure the new changes I made did not introduce new bugs or decrease the integrity of the codebase. I will be careful when the pull request is reviewed.