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

Add spec for cluster stats metric and index_metric filter paths. #639

Conversation

SwethaGuptha
Copy link
Contributor

Description

Adding specs for path /_cluster/stats/{metric}/nodes/{nodeId} and /_cluster/stats/{metric}/{index_metric}/nodes/{nodeId} in cluster/stats API. With opensearch-project/OpenSearch#15938, the cluster/stats API will have filtering support that will allow the clients to fetch only specific metrics from cluster/stats instead of all metrics.

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...].

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Contributor

github-actions bot commented Oct 24, 2024

Changes Analysis

Commit SHA: 3ceb510
Comparing To SHA: fce5721

API Changes

Summary

├─┬Paths
│ ├──[➕] path (1840:3)
│ └──[➕] path (1862:3)
└─┬Components
  ├──[➕] parameters (17278:7)
  ├──[➕] parameters (17288:7)
  ├──[➕] schemas (47763:7)
  ├──[➕] schemas (47792:7)
  ├─┬cluster.stats___StatsResponseBase
  │ ├─┬ALLOF
  │ │ └──[🔀] $ref (52964:13)❌ 
  │ └─┬ALLOF
  │   └──[🔀] $ref (47850:11)❌ 
  ├─┬cluster.stats___ClusterNodes
  │ ├──[➖] required (47479:11)❌ 
  │ ├──[➖] required (47480:11)❌ 
  │ ├──[➖] required (47483:11)❌ 
  │ ├──[➖] required (47485:11)❌ 
  │ ├──[➖] required (47486:11)❌ 
  │ ├──[➖] required (47487:11)❌ 
  │ ├──[➖] required (47478:11)❌ 
  │ ├──[➖] required (47481:11)❌ 
  │ ├──[➖] required (47482:11)❌ 
  │ ├──[➖] required (47484:11)❌ 
  │ └──[➖] required (47477:11)❌ 
  └─┬cluster.stats___ClusterIndices
    ├──[➖] required (47263:11)❌ 
    ├──[➖] required (47261:11)❌ 
    ├──[➖] required (47262:11)❌ 
    ├──[➖] required (47265:11)❌ 
    ├──[➖] required (47266:11)❌ 
    ├──[➖] required (47267:11)❌ 
    ├──[➖] required (47268:11)❌ 
    ├──[➖] required (47269:11)❌ 
    ├──[➖] required (47260:11)❌ 
    └──[➖] required (47264:11)❌ 

Document Element Total Changes Breaking Changes
components 27 25
paths 2 0
  • BREAKING Changes: 25 out of 29
  • Modifications: 2
  • Removals: 21
  • Additions: 6
  • Breaking Removals: 21
  • Breaking Modifications: 2

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/11826040133/artifacts/2184467167

API Coverage

Before After Δ
Covered (%) 588 (57.59 %) 588 (57.59 %) 0 (0 %)
Uncovered (%) 433 (42.41 %) 433 (42.41 %) 0 (0 %)
Unknown 40 42 2

@SwethaGuptha SwethaGuptha force-pushed the api_spec_cluster_stats_metric_filtering branch 3 times, most recently from 4c500a7 to d0602a5 Compare October 24, 2024 07:40
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Looking good.

  1. Bump the SHA for a more recent build of 2.18 and 3.0 in https://github.com/opensearch-project/opensearch-api-specification/blob/main/.github/workflows/test-spec.yml#L45 until tests pass.
  2. Add to CHANGELOG.

@dblock
Copy link
Member

dblock commented Oct 24, 2024

@SwethaGuptha Looks like the build has other problems (missing other plugins like SQL and ISM). This is a known and rather annoying problem, 3.x builds don't always have everything. You should chase what's broken in OpenSearch main distribution.

I opened opensearch-project/opensearch-build#5130 to see what the infra team says about this (cc: @gaiksaya).

@gaiksaya

@SwethaGuptha
Copy link
Contributor Author

@SwethaGuptha Looks like the build has other problems (missing other plugins like SQL and ISM). This is a known and rather annoying problem, 3.x builds don't always have everything. You should chase what's broken in OpenSearch main distribution.

I opened opensearch-project/opensearch-build#5130 to see what the infra team says about this (cc: @gaiksaya).

@gaiksaya

Thanks @dblock for creating the issue. Did the tests work for v#3.0.0 previously? I see there are some failures from core OS as well. Also,I see the latest image available for v#3.0 is from eight days ago, do you know what cadence is followed for updating the version image?

@dblock
Copy link
Member

dblock commented Oct 24, 2024

Thanks @dblock for creating the issue. Did the tests work for v#3.0.0 previously? I see there are some failures from core OS as well.

Yes, the failure on main right now came from #625 and will get fixed soon.

Also,I see the latest image available for v#3.0 is from eight days ago, do you know what cadence is followed for updating the version image?

I think they try to build one daily, but it doesn't always work :(

@gaiksaya
Copy link
Member

I opened opensearch-project/opensearch-build#5130 to see what the infra team says about this (cc: @gaiksaya).

@gaiksaya

Will add more details to the issue but docker image is build using tarball. And all the distributions contains distribution manifest with all the information about a distribution and the components bundled in that distribution.

@dblock
Copy link
Member

dblock commented Oct 24, 2024

I opened opensearch-project/opensearch-build#5130 to see what the infra team says about this (cc: @gaiksaya).
@gaiksaya

Will add more details to the issue but docker image is build using tarball. And all the distributions contains distribution manifest with all the information about a distribution and the components bundled in that distribution.

It's just hard to tell from a distribution what the docker SHA is.

@SwethaGuptha
Copy link
Contributor Author

@dblock Any inputs on the next steps to make progress on this PR?

@dblock
Copy link
Member

dblock commented Oct 29, 2024

@SwethaGuptha yes, update the SHAs to the latest available for 2.18 and 3.0 and see if all tests pass - it has to pass 2.18, but you can mark tests as < 3.0 if that one doesn't pass, for now

@dblock
Copy link
Member

dblock commented Nov 7, 2024

Now that 2.18 has been released this should be easier.

@SwethaGuptha SwethaGuptha force-pushed the api_spec_cluster_stats_metric_filtering branch 2 times, most recently from afd68b1 to 975727e Compare November 13, 2024 17:08
Copy link
Contributor

github-actions bot commented Nov 13, 2024

Spec Test Coverage Analysis

Total Tested
517 349 (67.5 %)

@SwethaGuptha
Copy link
Contributor Author

@dblock Can you please help with the review?

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Looks good, some cosmetic asks, please.

CHANGELOG.md Outdated Show resolved Hide resolved
spec/schemas/cluster.stats.yaml Outdated Show resolved Hide resolved
spec/schemas/cluster.stats.yaml Outdated Show resolved Hide resolved
@SwethaGuptha SwethaGuptha force-pushed the api_spec_cluster_stats_metric_filtering branch from 975727e to 7103864 Compare November 13, 2024 19:03
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Sorry to be annoying, some more small stuff flagged here.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
spec/namespaces/cluster.yaml Outdated Show resolved Hide resolved
Signed-off-by: Swetha Guptha <gupthasg@amazon.com>
@SwethaGuptha SwethaGuptha force-pushed the api_spec_cluster_stats_metric_filtering branch from 60ba9ec to 3ceb510 Compare November 13, 2024 21:22
@dblock dblock merged commit 6bb1fed into opensearch-project:main Nov 13, 2024
24 checks passed
@dblock
Copy link
Member

dblock commented Nov 13, 2024

Thanks for taking this on, merged!

There's also node stats, do those have to mirror cluster stats?

 GET /_nodes/{node_id}/stats/{metric}/{index_metric}
 GET /_nodes/{node_id}/usage
 GET /_nodes/{node_id}/usage/{metric}
 GET /_nodes/{node_id}/{metric}

@nhtruong
Copy link
Collaborator

nhtruong commented Nov 14, 2024

@dblock
So cluster.stats will have the following paths:

  • /_cluster/stats
  • /_cluster/stats/nodes/{node_id}
  • /_cluster/stats/{metric}/nodes/{node_id}
  • /_cluster/stats/{metric}/{index_metric}/nodes/{node_id}

current path-finding strat for the JS CodeGen will create:

  const path = ['/_cluster/stats/', metric, '/', index_metric, '/nodes/', node_id].filter(c => c).join('').replace('//', '/');

which works fine for most situations EXCEPT for when no path params are provided since it will yield /_cluster/stats/nodes, which is not a valid path. I've gotta update the CodeGen for JS (I really like the above solution if not for this edge case 😢 )
cc: @Xtansia in case this affects Java/.NET CodeGen too.

@dblock
Copy link
Member

dblock commented Nov 14, 2024

@nhtruong I think these are 2 separate APIs, one is cluster stats (/_cluster/stats), and the other is cluster/nodes stats (the other 3), would that help?

@nhtruong
Copy link
Collaborator

nhtruong commented Nov 14, 2024

I'm referring to these 4 operations: https://github.com/opensearch-project/opensearch-api-specification/pull/639/files#diff-c68dc8d4bd22aabd667fe0978fba9a121988d77ac766b1e2a5e94091603c0734R305-R366

It's no big deal. We just can't have that one-line solution 🥲

@dblock
Copy link
Member

dblock commented Nov 14, 2024

@nhtruong Yes, I think they should be totally different operations, no? would that not work?

@nhtruong
Copy link
Collaborator

nhtruong commented Nov 14, 2024

They are in the same operation-group which means they will be piped into the same API function on the client. That function has to pick a path depending on the path params provided by the user. NodeJS CodeGen is using that one-line solution which works for the vast majority of the operation groups, even the notorious nodes.info:

  • /_nodes
  • /_nodes/{node_id}
  • /_nodes/{metric}
  • /_nodes/{node_id}/{metric}

const path = ['/_nodes/', node_id, '/', metric].filter(c => c).join('').replace('//', '/');

It's not hard to switch up the path selection logic to account for this edge case.

@dblock
Copy link
Member

dblock commented Nov 14, 2024

They are in the same operation-group

Is this correct?

@nhtruong
Copy link
Collaborator

Yes, the 4 operations I stated in #639 (comment) are in cluster.stats

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants