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

Move default hostname logic from the aggregator to the sender and dsd #2334

Merged
merged 14 commits into from
Sep 28, 2018

Conversation

xvello
Copy link
Contributor

@xvello xvello commented Sep 18, 2018

What does this PR do?

In order to allow cluster checks to submit metrics / events / servicechecks with an empty hostname, this PR moves the default hostname injection logic responsibility from the aggregator to the sender / source:

  • remove the default hostname logic from the checkSampler (for metrics) and the BufferedAggregator (for events / SCs)
  • remove the unused defaultHostname field from distSampler and timeSampler
  • add the hostname injection logic to the checkSender itself, allow checks to disable it via the new DisableDefaultHostname method
  • add default hostname enforcing to dogstatsd's event and SCs parsing logic (as it was already done for metrics). Add handling of the dummy host: tag to remove the hostname, as is done with metrics.

The collector part will come in a second PR.

@xvello xvello added this to the 6.6.0 milestone Sep 18, 2018
@xvello xvello requested a review from a team as a code owner September 18, 2018 15:45
@xvello xvello requested a review from a team as a code owner September 19, 2018 10:03
@xvello xvello changed the title [WIP] Move default hostname logic from the aggregator to the sender Move default hostname logic from the aggregator to the sender Sep 21, 2018
@xvello xvello changed the title Move default hostname logic from the aggregator to the sender Move default hostname logic from the aggregator to the sender and dsd Sep 21, 2018
hush-hush
hush-hush previously approved these changes Sep 21, 2018
Copy link
Member

@hush-hush hush-hush left a comment

Choose a reason for hiding this comment

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

this is 👍 !

- move the default hostname responsibility from the aggregator / sampler
to the sender
- add a DisableDefaultHostname method to the Sender interface
- make dogstatsd handle hostname for events and service checks
This reverts commit a99fc330ad7ae60c18ab21071428008fe3747b2b.
@xvello
Copy link
Contributor Author

xvello commented Sep 27, 2018

Added end-to-end testing, to ensure dsd hostname cases are handled. They'll be extended on #2351 to add a check with default hostname disabled too

hush-hush
hush-hush previously approved these changes Sep 27, 2018
mfpierre
mfpierre previously approved these changes Sep 28, 2018

assert.Equal(t, "testhostname", checksender.defaultHostname)
assert.Equal(t, false, checksender.defaultHostnameDisabled)

Copy link
Contributor

Choose a reason for hiding this comment

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

Extra line here

@xvello xvello dismissed stale reviews from mfpierre and hush-hush via 5d5bda4 September 28, 2018 09:59
@xvello xvello merged commit cad9f0c into master Sep 28, 2018
@albertvaka albertvaka deleted the xvello/nohostname branch July 8, 2019 10:24
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