-
Notifications
You must be signed in to change notification settings - Fork 305
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
[telemetry] optionally decouple telemetry destination from other metrics #558
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment, LGTM otherwise
datadog/dogstatsd/base.py
Outdated
telemetry_min_flush_interval=DEFAULT_TELEMETRY_MIN_FLUSH_INTERVAL, | ||
telemetry_host=None, telemetry_port=DEFAULT_PORT, telemetry_socket_path=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we document the added params in the docstring please ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely, my bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use None too for the port, for the case where users change the port and want the telemetry to follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One quick typo, and lgtm
datadog/dogstatsd/base.py
Outdated
telemetry_min_flush_interval=DEFAULT_TELEMETRY_MIN_FLUSH_INTERVAL, | ||
telemetry_host=None, telemetry_port=DEFAULT_PORT, telemetry_socket_path=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use None too for the port, for the case where users change the port and want the telemetry to follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last nit, looks good otherwise.
This issue has been automatically marked as stale because it has not had activity in the last 30 days. |
datadog/dogstatsd/base.py
Outdated
|
||
:envvar DD_TELEMETRY_PORT: the port for the dogstatsd server we wish to submit | ||
telemetry stats to. If set, it overrides default value. | ||
:type DD_TELEMETRY_HOST: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be DD_TELEMETRY_PORT
in place of DD_TELEMETRY_HOST
here, no ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're 100% right, thanks for pointing it out. We're bringing this PR back to life, hopefully we'll merge it soon.
Co-Authored-By: Hippolyte HENRY <zippolyte@users.noreply.github.com>
c4428a0
to
84f1971
Compare
What does this PR do?
In some esoteric setups or testing it might be useful to submit telemetry statistics to a different dogstatsd server.
Description of the Change
This PR allows to set a different destination for telemetry metrics flushed.
Alternate Designs
No alternate designs were considered.
Possible Drawbacks
Added complexity to Dogstatsd constructor, could be confusing if not well documented.
Verification Process
Unit tests updated. Feature is being used for dogstatsd performance/feature testing currently.
Additional Notes
None.
Release Notes
Review checklist (to be filled by reviewers)
changelog/
label attached. If applicable it should have thebackward-incompatible
label attached.do-not-merge/
label attached.kind/
andseverity/
labels attached at least.