Skip to content

Conversation

szybia
Copy link
Contributor

@szybia szybia commented Sep 30, 2025

Server PRs: elastic/elasticsearch#134835 + elastic/elasticsearch#135271

@szybia szybia added the skip-backport This pull request should not be backported label Sep 30, 2025
Copy link
Contributor

github-actions bot commented Sep 30, 2025

Following you can find the validation changes against the target branch for the APIs.

API Status Request Response
indices.create 🔴 1339/1372 → 1342/1372 1372/1372
indices.put_mapping 🔴 138/145 → 139/145 148/148
security.get_stats 🟠 → 🟢 Missing type → 2/2 Missing type → 2/2

You can validate these APIs yourself by using the make validate target.

@szybia
Copy link
Contributor Author

szybia commented Sep 30, 2025

let me fix validate failure ^ in next PR to keep PRs scoped to one purpose, i have it passing locally

security.get_stats request has been successfully validated!
✔ 2 out of 2 test request cases are passing for security.get_stats.
✔ 2 out of 2 test response cases are passing for security.get_stats.

@szybia szybia marked this pull request as ready for review September 30, 2025 12:00
@szybia szybia requested review from joegallo and pquentin September 30, 2025 12:00
@pquentin
Copy link
Member

Thank you for this!

let me fix validate failure ^ in next PR to keep PRs scoped to one purpose, i have it passing locally

I'm glad that you got make validate passing! It's not always easy. Did it require a big/unrelated change? I'd rather review something that passes the test.

@szybia
Copy link
Contributor Author

szybia commented Sep 30, 2025

nope change is quite small

i can put it into this PR if you prefer, i was thinking cleaner to have two PRs add new endpoint and update DLS cache stats to have new properties

change:

diff --git a/specification/xpack/usage/types.ts b/specification/xpack/usage/types.ts
index 085fff1ed..e5ad6ac55 100644
--- a/specification/xpack/usage/types.ts
+++ b/specification/xpack/usage/types.ts
@@ -320,9 +320,42 @@ export class SecurityRolesDls {
 }

 export class SecurityRolesDlsBitSetCache {
+  /** Number of entries in the cache. */
   count: integer
+  /** Human-readable amount of memory taken up by the cache. */
   memory?: ByteSize
+  /** Memory taken up by the cache in bytes. */
   memory_in_bytes: ulong
+  /**
+   * Total number of cache hits.
+   * @availability stack since=9.2.0
+   * @availability serverless
+   */
+  hits: ulong
+  /**
+   * Total number of cache misses.
+   * @availability stack since=9.2.0
+   * @availability serverless
+   */
+  misses: ulong
+  /**
+   * Total number of cache evictions.
+   * @availability stack since=9.2.0
+   * @availability serverless
+   */
+  evictions: ulong
+  /**
+   * Total combined time spent in cache for hits in milliseconds.
+   * @availability stack since=9.2.0
+   * @availability serverless
+   */
+  hits_time_in_millis: ulong
+  /**
+   * Total combined time spent in cache for misses in milliseconds.
+   * @availability stack since=9.2.0
+   * @availability serverless
+   */
+  misses_time_in_millis: ulong
 }

@pquentin
Copy link
Member

Including this change is fine, thank you! It's quite valuable to see the green report. Plus the SecurityRolesDlsBitSetCache changes may affect other APIs, which could affect this PR too.

@szybia szybia changed the title Add /_security/stats specification Security: Add new endpoint and DLS properties Sep 30, 2025
@joegallo joegallo requested a review from masseyke September 30, 2025 13:58
Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thank you! A few minor adjustments are needed.

* @availability stack since=9.2.0
* @availability serverless
*/
hits: ulong
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize the above uses ulong, but let's still stick to long for the new values since this is the actual Java type.

Suggested change
hits: ulong
hits: long

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

i was thinking ulong to indicate this can't be < 0 even tho java type is long; but it all gets turned to a number i believe so probably doesn't matter

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I discussed it with the team and it's... complicated. We only have 4 of those right now, and hundreds of long that are also going to be positive-only. Their handling in the different generators is unclear. It's very minor, so let's not get blocked on this.

I did revert the change to the existing ulong in 6c507dee since that would have been a breaking change.

@szybia
Copy link
Contributor Author

szybia commented Oct 2, 2025

just flagging in case

but when changing {hits,misses}_time_in_millis to DurationValue<UnitMillis>, it stops appearing on the bump preview ./output/openapi/elasticsearch-openapi.json

we already use this everywhere so assuming whatever we use to actually generate the docs does this correctly

@szybia szybia requested a review from pquentin October 2, 2025 10:43
@pquentin
Copy link
Member

pquentin commented Oct 2, 2025

just flagging in case

Wow, nice catch! I alerted the docs team.

Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your attention to detail! LGTM.

@pquentin pquentin added backport 9.2 and removed skip-backport This pull request should not be backported labels Oct 2, 2025
@pquentin pquentin merged commit 3e3c203 into main Oct 2, 2025
11 checks passed
@pquentin pquentin deleted the security-stats-endpoint branch October 2, 2025 12:39
github-actions bot pushed a commit that referenced this pull request Oct 2, 2025
* Add `/_security/stats` specification

* Add new properties to DLS cache stats

* quentin comments

* Revert breaking ulong change

---------

Co-authored-by: Quentin Pradet <quentin.pradet@elastic.co>
(cherry picked from commit 3e3c203)
pquentin added a commit that referenced this pull request Oct 2, 2025
* Add `/_security/stats` specification

* Add new properties to DLS cache stats

* quentin comments

* Revert breaking ulong change

---------


(cherry picked from commit 3e3c203)

Co-authored-by: Szymon Bialkowski <szymon.bialkowski@elastic.co>
Co-authored-by: Quentin Pradet <quentin.pradet@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants