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: report method name in WS metrics #29

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mkykadir
Copy link

Description

Access method name in WS context so metrics can be reported with method name

@mkykadir mkykadir requested a review from a team as a code owner July 19, 2024 12:45
@mkykadir mkykadir requested a review from zhwrd July 19, 2024 12:45
@jelias2
Copy link
Contributor

jelias2 commented Jul 26, 2024

Hi Mkykadir,

Thanks for the contribution!

Its a clever solution, however I am concerned about a race condition occurring with concurrent map access w.reqIdToMethod if the proxyd was under heavy ws traffic.

I'm curious if you would be able to find a solution that can avoid this?

Also please include test cases if possible

Copy link
Contributor

@jelias2 jelias2 left a comment

Choose a reason for hiding this comment

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

In the PRs current state with concurrent map access, I believe these concerns must be addressed before merging

@mkykadir
Copy link
Author

mkykadir commented Aug 5, 2024

@jelias2 nice catches actually I missed a crucial point as well:

  • reqId within context is determined on WS connection and shouldn't be used for all following ws messages
  • now each ws connection will have its own concurrent map
  • and following message ids will be used for this
  • after its ok for you I'll go with tests as well

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.

2 participants