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

Fix: remove the check for the Server header of the Instana agent. #427

Merged
merged 1 commit into from
May 25, 2023

Conversation

pvital
Copy link
Member

@pvital pvital commented May 22, 2023

The check for the Server header of the Instana agent can break the announce procedure in service meshes when there is a proxy between the tracer and the Instana agent. This happens because some proxies do not forward the original Server header. So instead, the collector checks whether the HTTP status code is in the 2xx range.

@pvital pvital requested a review from Ferenc- May 22, 2023 15:07
@pvital pvital self-assigned this May 22, 2023
@pvital pvital requested a review from basti1302 May 22, 2023 15:07
Copy link
Contributor

@basti1302 basti1302 left a comment

Choose a reason for hiding this comment

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

Looks good. We should double check the timeout change, see inline comment.

instana/agent/host.py Outdated Show resolved Hide resolved
@pvital pvital force-pushed the remove_check_for_server_header branch from e0ec53c to 17d576d Compare May 22, 2023 15:43
@basti1302 basti1302 self-requested a review May 22, 2023 15:49
Copy link
Collaborator

@Ferenc- Ferenc- left a comment

Choose a reason for hiding this comment

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

Good work so far. We are getting there.
The coverage report shows that the failed attempt is nicely covered with the test case,
while the successful attempt is not yet covered.

Although the production code around the succesful attempt is minuscule.
I still think covering the succesful attempt would be valuable, not just because we can't have a functioning system without a successful connection attempts,
but also because this change is about enabling successful connections in a broader situation.
so any other change is secondary priority here.

tests/platforms/test_host.py Outdated Show resolved Hide resolved
@pvital pvital force-pushed the remove_check_for_server_header branch from 17d576d to f9a8988 Compare May 24, 2023 06:09
@pvital pvital requested a review from Ferenc- May 24, 2023 06:09
The check for the Server header of the Instana agent can break the announce
procedure in service meshes when there is a proxy between the tracer and the
Instana agent. This happens because some proxies do not forward the original
Server header. So instead, the collector checks whether the HTTP status code
is in the 2xx range.

Signed-off-by: Paulo Vital <paulo.vital@ibm.com>
@pvital pvital force-pushed the remove_check_for_server_header branch from f9a8988 to 4211415 Compare May 25, 2023 09:06
Copy link
Collaborator

@Ferenc- Ferenc- left a comment

Choose a reason for hiding this comment

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

Approved

@Ferenc- Ferenc- merged commit e0d5bc5 into master May 25, 2023
@Ferenc- Ferenc- deleted the remove_check_for_server_header branch May 25, 2023 09:34
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.

3 participants