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 incorrect configuration mismatch warning when port is specified as String #1720

Merged
merged 3 commits into from
Oct 18, 2021

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Oct 15, 2021

I noticed a Ruby app where the configuration mismatch warning was popping up:

W, [2021-10-15T11:44:30.952540 #1]  WARN -- ddtrace: [ddtrace] Configuration mismatch: values differ between "c.tracer.port" ('8126') and DD_TRACE_AGENT_PORT environment variable ('8126'). Using '8126'.

The warning was being triggered because we convert the values read from environment variables (DD_TRACE_AGENT_PORT or DD_TRACE_AGENT_URL) into integers, but this app had c.tracer.port specified as a string.

Because or that, since we collect all values, call #uniq on them and see if we get more than one, the warning was being triggered since the 8126 integer is not the same as the '8126' string.

…s String

I noticed a Ruby app where the configuration mismatch warning was
popping up:

```
W, [2021-10-15T11:44:30.952540 #1]  WARN -- ddtrace: [ddtrace]
Configuration mismatch: values differ between "c.tracer.port" ('8126')
and DD_TRACE_AGENT_PORT environment variable ('8126'). Using '8126'.
```

The warning was being triggered because we convert the values read from
environment variables (`DD_TRACE_AGENT_PORT` or `DD_TRACE_AGENT_URL`)
into integers, but this app had `c.tracer.port` specified as a
string.

Because or that, since we collect all values, call `#uniq` on them
and see if we get more than one, the warning was being triggered
since the `8126` integer is not the same as the `'8126'` string.
@ivoanjo ivoanjo requested a review from a team October 15, 2021 18:22
@ivoanjo ivoanjo changed the title git logIvoanjo/fix warning when port specified as string Fix incorrect configuration mismatch warning when port is specified as String Oct 15, 2021
@ivoanjo
Copy link
Member Author

ivoanjo commented Oct 15, 2021

You can tell it's friday evening because I opened a PR called "git logIvoanjo/fix warning when port specified as string" due to not correctly focusing my terminal 🤣

Turns out `Integer(...)` throws `ArgumentError` when a string doesn't
make sense, but `TypeError` when what it gets isn't even a string.
@codecov-commenter
Copy link

Codecov Report

Merging #1720 (9c0c364) into master (8f697cc) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1720   +/-   ##
=======================================
  Coverage   98.18%   98.18%           
=======================================
  Files         934      934           
  Lines       45122    45150   +28     
=======================================
+ Hits        44301    44329   +28     
  Misses        821      821           
Impacted Files Coverage Δ
...b/ddtrace/configuration/agent_settings_resolver.rb 100.00% <100.00%> (ø)
...race/configuration/agent_settings_resolver_spec.rb 100.00% <100.00%> (ø)
spec/ddtrace/integration_spec.rb 97.16% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f697cc...9c0c364. Read the comment docs.

@ivoanjo ivoanjo merged commit a953eb0 into master Oct 18, 2021
@ivoanjo ivoanjo deleted the ivoanjo/fix-warning-when-port-specified-as-string branch October 18, 2021 09:12
@github-actions github-actions bot added this to the 0.54.0 milestone Oct 18, 2021
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.

3 participants