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: Add channel statistics data return command #1494

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

qaqhy
Copy link

@qaqhy qaqhy commented Jul 29, 2024

API consumers can query channel statistics information
image

@mreiferson
Copy link
Member

I think this seems reasonable, thanks for submitting! Why didn't you use the client.Stats(topic) function to pull everything relevant to the client, though?

@qaqhy
Copy link
Author

qaqhy commented Sep 5, 2024

The client.Stats (topic) method cannot retrieve the information of client_countt, clients, and e2e_processing_1atency, and it is unknown whether this information is suitable to be returned together
image

@qaqhy
Copy link
Author

qaqhy commented Sep 5, 2024

I think this seems reasonable, thanks for submitting! Why didn't you use the client.Stats(topic) function to pull everything relevant to the client, though?

The HTTP method requires initiating a request, and in practical use, it is more convenient to directly use the Consumer object

@mreiferson
Copy link
Member

I thought these were client stats, these are actually channel stats. What's the thinking here? Regardless of the semantics of the STATS command, we should use existing methods of safely locking and pulling state rather than duplicating that code directly in this command handler (e.g. NewChannelStats() in this case).

(FWIW the client.Stats function does not perform an HTTP request).

@qaqhy
Copy link
Author

qaqhy commented Oct 8, 2024

I thought these were client stats, these are actually channel stats. What's the thinking here? Regardless of the semantics of the STATS command, we should use existing methods of safely locking and pulling state rather than duplicating that code directly in this command handler (e.g. NewChannelStats() in this case).

(FWIW the client.Stats function does not perform an HTTP request).

I read the NewChannelStats() method, but in the STATS method, it retrieves the statistics of the subscribed channel as a client. I believe that in my client connection, it shouldn't be able to access information about other clients (as this doesn't seem to be public information). If I call it like NewChannelStats(client.Channel, nil, 0), the client_count and clients would be incorrect. In reality, I don't want the client to know the client_count and clients data.

@mreiferson
Copy link
Member

I still think we should improve the API for how we get channel-related stats for an individual client, rather than duplicating the code and making this method aware of the concurrency and locking necessary to do it correctly.

@qaqhy
Copy link
Author

qaqhy commented Nov 18, 2024

I still think we should improve the API for how we get channel-related stats for an individual client, rather than duplicating the code and making this method aware of the concurrency and locking necessary to do it correctly.

The main data we want to obtain from this API are depth and backend_depth. The other data are additional and can be omitted. This way, we can avoid locking and unlocking operations in the Stats method.

@qaqhy
Copy link
Author

qaqhy commented Nov 18, 2024

I still think we should improve the API for how we get channel-related stats for an individual client, rather than duplicating the code and making this method aware of the concurrency and locking necessary to do it correctly.

The main data we want to obtain from this API are depth and backend_depth. The other data are additional and can be omitted. This way, we can avoid locking and unlocking operations in the Stats method.

We want the connected client to be able to check how many tasks are left to consume in the subscribed channel, which will help us predict the remaining consumption time for our business purposes. We prefer not to query this information via HTTP; using TCP within the business code would be more convenient.

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

Successfully merging this pull request may close these issues.

2 participants