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

Allow logging the behaviour of middleware #248

Closed
wants to merge 2 commits into from

Conversation

benlangfeld
Copy link
Collaborator

In a consuming application, when debugging slow responses to subscribing clients, it is useful to know more detail about how message_bus handled the request.

Tracking some attributes of a connection will allow us to spot patterns in use of long polling/chunked encoding, compare response times to subscription volume, etc.

Our use case involves NewRelic::Agent.add_custom_attributes but the implementation here is agnostic of the tool used to record this data.

This was a part of #243, and I think the less controversial part. I'm still working on getting something to satisfy the rest of that PR's feedback ready, but in the meantime maybe this can ship.

In a consuming application, when debugging slow responses to subscribing clients, it is useful to know more detail about how message_bus handled the request.

Tracking some attributes of a connection will allow us to spot patterns in use of long polling/chunked encoding, compare response times to subscription volume, etc.

Our use case involves `NewRelic::Agent.add_custom_attributes` but the implementation here is agnostic of the tool used to record this data.
@benlangfeld benlangfeld requested a review from SamSaffron April 30, 2021 19:19
@SamSaffron
Copy link
Member

I follow, but I kind of want to wait on us establishing the pattern in #243, it is possible a simple method extraction here would do the trick without needing to add the custom callback.

@benlangfeld
Copy link
Collaborator Author

I follow, but I kind of want to wait on us establishing the pattern in #243, it is possible a simple method extraction here would do the trick without needing to add the custom callback.

@SamSaffron Our tests revealed that our focus particularly should be within Client#backlog, and I don't personally anticipate needing to move forward with the rest of #243. I'll take a look at what changes are necessary to allow instrumentation of Client#backlog.

@SamSaffron
Copy link
Member

This has been hanging so long @benlangfeld, I am going to close this off for now.

@@ -139,6 +139,20 @@ def handle_request(env)

backlog = client.backlog

if @bus.on_middleware_attributes
@bus.on_middleware_attributes.call(
messagebus_seq: client.seq,
Copy link
Member

Choose a reason for hiding this comment

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

I think this is my biggest issue with this change, would prefer if we just pass env along and a fatter object vs all these options.

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

Successfully merging this pull request may close these issues.

2 participants