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

Ensure client includes AgentDescription on Reconnect #63

Merged
merged 2 commits into from
Apr 13, 2022

Conversation

dsvanlani
Copy link
Contributor

Resolves: #55

This PR ensures that the client includes AgentDescription every time it connects to the server, not just after client.Start.

Current behavior:

  • Clients connection to server is interrupted.
  • Client reconnects.
  • Client does not prepare a "first status message" with an AgentDescription.

Expected behavior:

  • Clients connection to server is interrupted.
  • Client reconnects.
  • Client prepares and sends a status including AgentDescription.

@dsvanlani dsvanlani requested a review from a team April 13, 2022 18:27
Copy link
Contributor

@andykellr andykellr left a comment

Choose a reason for hiding this comment

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

I collaborated with @dsvanlani on this fix so I would appreciate another review.

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM

@tigrannajaryan tigrannajaryan merged commit 2031210 into open-telemetry:main Apr 13, 2022
@tigrannajaryan
Copy link
Member

Thanks for the fix.

@andykellr andykellr deleted the send-status-on-reconnect branch April 13, 2022 19:10
@tigrannajaryan
Copy link
Member

I may have been too quick to merge this.

Re-reading the spec, it says:

The Agent MUST send a status report:
* First time immediately after connecting to the Server. The status report MUST
  be the first message sent by the Agent.
* Subsequently every time the status of the Agent changes.

Nowhere does it says that the AgentDescription must be also included in the status report. On the contrary, the spec says:

<h4 id="agent_description">agent_description</h4>

The description of the agent, its type, where it runs, etc. See
[AgentDescription](#agentdescription-message) message for details.

This field SHOULD be unset if no Agent description fields have changed since the
last StatusReport was sent.

So, this fix it correct that it sends the status report after reconnect (it was a bug that we didn't), but we now unnecessarily include the AgentDescription in these subsequent status reports.

@dsvanlani Can you do a follow-up PR that corrects this behavior?

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.

Client should send status report on reconnect
3 participants