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

Account for control character as part of zmq_read_part #906

Merged
merged 4 commits into from
Aug 29, 2024

Conversation

zbud-msft
Copy link
Contributor

ADO: 28728116

Capturing control character in do_capture as part of eventd is difficult as control character can be delayed such that we it is not sometimes caught when initially starting capture service. (SUB -> PUB (part of zmq proxy)).

Instead of dropping the control character as part of eventd do_capture, I will drop it as part of zmq_message_read which eventd calls via events_service.

  1. Check rc value of zmq_msg_recv
  2. if rc == 1 (meaning 1 byte long)
  3. verify it is control character, log that we received subscription messages
  4. drop it such that empty event is sent (empty events are dropped by eventd)

LIBZMQ: Control character logic: https://github.com/zeromq/libzmq/blob/cbb9925a109152426e4a8a3f8f8eaeddc0516977/src/xsub.cpp#L102

Added UT to confirm, as well as should see success rate of sonic-mgmt testcases go up for telemetry/test_events

common/events_common.h Outdated Show resolved Hide resolved
@zbud-msft zbud-msft merged commit 636f565 into sonic-net:master Aug 29, 2024
17 checks passed
@zbud-msft
Copy link
Contributor Author

@StormLiangMS Can you please review for 202311?

@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202311: #910

@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202405: #911

@mssonicbld
Copy link
Collaborator

@zbud-msft cherry pick PR didn't pass PR checker. Please check!!! Auto cherry pick PR will be closed in 3 days.
#911

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

Successfully merging this pull request may close these issues.

5 participants