Skip to content

feat: Add channel statistics data return command#1494

Open
qaqhy wants to merge 3 commits into
nsqio:masterfrom
qaqhy:channel_stats_command
Open

feat: Add channel statistics data return command#1494
qaqhy wants to merge 3 commits into
nsqio:masterfrom
qaqhy:channel_stats_command

Conversation

@qaqhy

@qaqhy qaqhy commented Jul 29, 2024

Copy link
Copy Markdown

API consumers can query channel statistics information
image

@mreiferson

Copy link
Copy Markdown
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

qaqhy commented Sep 5, 2024

Copy link
Copy Markdown
Author

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

qaqhy commented Sep 5, 2024

Copy link
Copy Markdown
Author

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
Copy Markdown
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

qaqhy commented Oct 8, 2024

Copy link
Copy Markdown
Author

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
Copy Markdown
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

qaqhy commented Nov 18, 2024

Copy link
Copy Markdown
Author

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

qaqhy commented Nov 18, 2024

Copy link
Copy Markdown
Author

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