-
Notifications
You must be signed in to change notification settings - Fork 306
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
Improve user-agent header to include telemetry information #583
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.
Looks good, two quick comments. Thanks!
tests/integration/api/test_api.py
Outdated
def test_user_agent(self, dog): | ||
_, resp = dog.api_client.APIClient.submit('GET', 'validate') | ||
assert re.match(r'^datadogpy\/[^\s]+ \(python [^\s]+; os [^\s]+; arch [^\s]+\)$', resp.request.headers['User-Agent']) | ||
resp.request.headers['User-Agent'] |
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.
Is this line needed?
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.
Ah, forgotten debugging line, sorry. Will remove.
@@ -11,9 +11,9 @@ envlist = | |||
passenv = DD_TEST_CLIENT* | |||
usedevelop = true | |||
deps = | |||
!integration: mock |
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.
I think if we move the user agent test to tests/unit instead of tests/integration we wouldn't need to change deps.
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.
Yeah, but the whole unittest suite as it is set up now replaces the RequestClient._session
, so we'd have to create a completely different setup just because of this one specific test. I don't mind doing that, but this is just simpler (plus it tests the real thing).
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
Hmm, I have also realized that there's a theoretical possibility that someone uses the |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
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.
LGTM
What does this PR do?
Makes the user-agent header more useful.
Description of the Change
Alternate Designs
Possible Drawbacks
Verification Process
Additional Notes
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.