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

ddtrace/tracer/option: fixing the struct for info endpoint response data #1803

Conversation

scott-shields-github
Copy link

What does this PR do?

This PR fixed the struct that is used to decode the response from the agent's /info endpoint. This fixes the issue reported in #1802 where the statsd port is returned in a nested struct.

Motivation

The trace library attempts to automatically load some information from the agent. Part of this information is the statsd port. Having the trace library auto configure the Dogstatsd port will prevent misconfigurations when using a non-default port.

Describe how to test/QA your changes

Run the updated tests.
Alternatively, create a tracer with an agent with a non-default statsd port. View the trace configuration logs to ensure the port is configured correctly.

Reviewer's Checklist

  • If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
  • Changed code has unit tests for its functionality.
  • If this interacts with the agent in a new way, a system test has been added.

@scott-shields-github scott-shields-github requested a review from a team March 15, 2023 03:35
Copy link
Contributor

@katiehockman katiehockman left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

@scott-shields-github
Copy link
Author

scott-shields-github commented Mar 20, 2023

@katiehockman Apologies, these tests are passing in my IDE because I don't have an agent running. I think these tests are failing here because there is an agent running and we are trusting that agent's reported port over user configured values.

I'm happy to fix the tests if that is still the expected behavior. I wonder if it would be better to trust the user input first, just incase they configure a different agent statsd agent address.

I'll wait for your confirmation before I begin updating, but I'd likely find a way to ignore the agent to test that path along with the expected path with an agent involved.

@ajgajg1134
Copy link
Contributor

@katiehockman Apologies, these tests are passing in my IDE because I don't have an agent running. I think these tests are failing here because there is an agent running and we are trusting that agent's reported port over user configured values.

I'm happy to fix the tests if that is still the expected behavior. I wonder if it would be better to trust the user input first, just incase they configure a different agent statsd agent address.

I'll wait for your confirmation before I begin updating, but I'd likely find a way to ignore the agent to test that path along with the expected path with an agent involved.

👋 I just took a look at this and yeah it seems that your fix here revealed that while we prefer the agent's reported port, that feature didn't actually work since we didn't parse it correctly. I do think that we want to stick with preferring the agent's reported value as called out in this comment. I think you're clear to go ahead with adding a way to ignore the agent to force testing the user-defined logic. Let me know if you'd like a hand at all on this :) Thanks again!

@katiehockman katiehockman added the stale Stuck for more than 1 month label Nov 28, 2023
@darccio darccio added do-not-merge/WIP and removed stale Stuck for more than 1 month labels Oct 1, 2024
@darccio darccio assigned darccio and hannahkm and unassigned darccio Oct 1, 2024
@hannahkm
Copy link
Contributor

The work for this PR is finished in #2931, so the related fix will go out in the next release. Thank you for your work!

@hannahkm hannahkm closed this Nov 12, 2024
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