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 client request to telemetry metadata #324

Merged
merged 2 commits into from
Nov 27, 2023

Conversation

ssepml
Copy link
Contributor

@ssepml ssepml commented Jul 4, 2023

Description

Having the actual client request in the telemetry event can be useful to cover more use cases, e.g. if you want to log specifics of the requests you are doing.

NOTE: Not sure if this was considered and discarded already so I just went ahead and created the PR as this would be helpful for my use case.

Copy link
Contributor

@polvalente polvalente left a comment

Choose a reason for hiding this comment

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

I like this. Could you add some assertions to one of the tests at test/grpc/integration/client_interceptor_test.exs showing the new metadata?

You can see the use of the custom attach_events helper there and follow the same pattern.

@ssepml ssepml force-pushed the telemetry_add_req branch 2 times, most recently from 5eca7e3 to d197e81 Compare July 6, 2023 08:03
@ssepml
Copy link
Contributor Author

ssepml commented Jul 6, 2023

@polvalente I added some asserts as you suggested. Though I only asserted some of the fields, specially for the stream. If you have something specific in mind, I can add it.

@ssepml
Copy link
Contributor Author

ssepml commented Jul 12, 2023

Just checking up, is there anything else I can do to move this forward?

@ssepml
Copy link
Contributor Author

ssepml commented Sep 12, 2023

Hi, any chance to get a second look at this? It would be great to have this included.

@ssepml
Copy link
Contributor Author

ssepml commented Sep 27, 2023

Hi @polvalente @sleipnir , thanks for the approvals! Just for my information, is there anything else that needs to happen before this gets merged?

@sleipnir
Copy link
Collaborator

sleipnir commented Oct 9, 2023

ping @polvalente

@polvalente
Copy link
Contributor

This is on my to-do list :)
Hopefully I'll get to it by next week.

@ssepml
Copy link
Contributor Author

ssepml commented Nov 14, 2023

@polvalente I apologize if I'm being pushy, but are there any news on this?

@polvalente polvalente merged commit 59b984c into elixir-grpc:master Nov 27, 2023
19 checks passed
@ssepml ssepml deleted the telemetry_add_req branch November 27, 2023 07:13
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.

3 participants