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

Add multiple_ips option and address tag #10868

Merged
merged 5 commits into from
Dec 17, 2021
Merged

Add multiple_ips option and address tag #10868

merged 5 commits into from
Dec 17, 2021

Conversation

coignetp
Copy link
Contributor

What does this PR do?

Add multiple_ips configuration option
When enabled, integration calls gethostbyname_ex (https://docs.python.org/3/library/socket.html#socket.gethostbyname_ex) instead of gethostbyname (https://docs.python.org/3/library/socket.html#socket.gethostbyname)

A domain can have multiple IPs attached, and gethostbyname returns only one of the IPs, whereas gethostbyname_ex returns all of them.

So when multiple_ips is enabled, we run the check against all the IPs attached to the domain instead of one

Motivation

Support case

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached

@codecov
Copy link

codecov bot commented Dec 15, 2021

Codecov Report

Merging #10868 (af85124) into master (f39f0d0) will increase coverage by 0.00%.
The diff coverage is 90.00%.

Flag Coverage Δ
tcp_check 89.83% <90.00%> (+0.99%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Comment on lines +125 to +126
check.check(None)
check.check(None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
check.check(None)
check.check(None)
dd_run_check(check)
dd_run_check(check)

small nit

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw that tcp_check still uses check.check(None) for its other non-e2e tests, so it's up to you if you want to apply this change or keep things consistent for a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's do this in another PR

yzhan289
yzhan289 previously approved these changes Dec 15, 2021
Copy link
Contributor

@sarah-witt sarah-witt left a comment

Choose a reason for hiding this comment

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

Looks great!

tcp_check/datadog_checks/tcp_check/tcp_check.py Outdated Show resolved Hide resolved
@coignetp coignetp changed the title Add multiple_ips option Add multiple_ips option and address tag Dec 15, 2021
@hithwen
Copy link
Contributor

hithwen commented Dec 16, 2021

A domain can have multiple IPs attached, and gethostbyname returns only one of the IPs, whereas gethostbyname_ex returns all of them.

Can we just always call gethostbyname_ex rather than adding a new config option?

@coignetp
Copy link
Contributor Author

Can we just always call gethostbyname_ex rather than adding a new config option?

That would break monitors and dashboards by default for customers aggregating by target_host without notice. If mydomain has 3 IPs, avg:network.tcp.can_connect{target_host:mydomain} value will completely change ; i think it's better to keep it as an option

@coignetp coignetp merged commit 0746f1e into master Dec 17, 2021
@coignetp coignetp deleted the paul/tcp-multiple branch December 17, 2021 09:52
cswatt pushed a commit that referenced this pull request Jan 5, 2022
* Add multiple_ips option

* Use gethostbyname_ex

* Add test

* Rename to resolve_ips

* Add address service check group
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