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

feat: adding filter data admin endpoint (REST) #2314

Merged
merged 10 commits into from
Jan 9, 2024

Conversation

gabrielmer
Copy link
Contributor

@gabrielmer gabrielmer commented Dec 21, 2023

Description

Adding admin REST endpoint to retrieve the serving node's subscriber filter clients and their subscribed pubsub and content topics.

Endpoint: GET /admin/v1/filter/subscriptions

Changes

  • mounted endpoint to retrieve filter subscriber data
  • added unit test verifying its proper functioning
  • updated openapi.yaml documentation

image

Issue

closes #2290

@gabrielmer gabrielmer self-assigned this Dec 21, 2023
@gabrielmer gabrielmer changed the title Feat adding filter data admin endpoint feat: adding filter data admin endpoint Dec 21, 2023
@gabrielmer gabrielmer changed the title feat: adding filter data admin endpoint feat: adding filter data admin endpoint (REST) Dec 21, 2023
Copy link

github-actions bot commented Dec 21, 2023

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2314

Built from 9e568f7

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

Wonderful PR! Thanks for it! 💯
nitpick: I wonder if we could add more test cases and not only the "happy path" :D

@gabrielmer
Copy link
Contributor Author

Wonderful PR! Thanks for it! 💯 nitpick: I wonder if we could add more test cases and not only the "happy path" :D

Oh yess, my bad! Copied from another endpoint, adapted it for this case and forgot about adding more 😳
Will add more comprehensive tests now! Thanks for the remark! 🤩

Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

Thanks for it, will be great for debugging as well.

Copy link
Contributor

@chaitanyaprem chaitanyaprem left a comment

Choose a reason for hiding this comment

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

Reviewed only the API details and left a minor comment.
LGTM once that is addressed.

waku/waku_api/rest/admin/openapi.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

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

LGTM Thank you!

I would add an error when filter is not mounted but it's ok for now.

@gabrielmer
Copy link
Contributor Author

LGTM Thank you!

I would add an error when filter is not mounted but it's ok for now.

Yes! I was 50-50 whether to return an error stating that filter isn't mounted or to return an empty array of subscribers in that case. I followed the second option just to be consistent with the approach of installAdminV1GetPeersHandler.

But I do agree that returning an error stating that the protocol is not even mounted is more informative. @Ivansete-status @NagyZoltanPeter, what do you think?

@NagyZoltanPeter
Copy link
Contributor

LGTM Thank you!
I would add an error when filter is not mounted but it's ok for now.

Yes! I was 50-50 whether to return an error stating that filter isn't mounted or to return an empty array of subscribers in that case. I followed the second option just to be consistent with the approach of installAdminV1GetPeersHandler.

But I do agree that returning an error stating that the protocol is not even mounted is more informative. @Ivansete-status @NagyZoltanPeter, what do you think?

My first thought is to return with error and correct explanation. Due this admin endpoint is strictly depends on Filter service to be mounted. Otherwise it has no meaning. Emtpy array would mean to me there is no subscriptions and Filter service is running.

@gabrielmer gabrielmer force-pushed the feat-adding-filter-data-admin-endpoint branch from 65f9dfc to 0012d60 Compare January 8, 2024 14:24
Copy link
Contributor

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

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

LGTM Thanks!


var subscriptions: seq[FilterSubscription] = @[]

if not node.wakuFilter.isNil():
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be remove since it's checked above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes! Was supposed to remove it but for sure got distracted and forgot 😶

Will fix it now, thanks so much!

Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

It's great!
Just to note to myself, I need to touch here as soon it gets merged - surely before my rework on managing filter subscriptions. that makes filter subscription structure upside-down. :-)

@gabrielmer gabrielmer merged commit bcab9d5 into master Jan 9, 2024
10 checks passed
@gabrielmer gabrielmer deleted the feat-adding-filter-data-admin-endpoint branch January 9, 2024 10:42
@SionoiS SionoiS added this to the Release 0.24.0 milestone Jan 9, 2024
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.

feat: Have additional Admin REST API endpoints that helps node operator in monitoring
5 participants