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
[core] Add HTTPS support #1055
[core] Add HTTPS support #1055
Conversation
f59d3b9
to
7f02d68
Compare
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.
@brettlangdon now you wish we were using URL ;)
rebasing on top of what got merged this morning (LMK if this is not necessary) EDIT: oof, didn't realize that would un-approve my PR. I should probably just leave this alone... |
(@berkowitzi fixed passing the |
tests/test_api.py
Outdated
api = API('localhost', 8126, '/path/to/uds') | ||
assert str(api) == '/path/to/uds' | ||
assert str(api) == 'http:///path/to/uds' |
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.
This should be unix:///path/to/uds
?
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.
that was my initial plan, but then how do you signify HTTPS + UDS? should it just not be allowed?
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.
https means HTTP over TLS. AFAIK, TLS runs normally on TCP.
So I don't think we should care about Unix Domain Socket + TLS.
By the way, unix:///path/to/uds
is not accurate. In fact, we speak HTTP over Unix Domain Socket. So we should probably write http+unix
or something.
unix:///path/to/uds
is an abbreviated notation.
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 just want to make sure that this PR is consistent with #1054.
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.
ok. I went with unix://
for both HTTP and HTTPS
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.
This looks good except the weird comment change :)
ddtrace/internal/writer.py
Outdated
@@ -193,8 +193,7 @@ def put(self, item): | |||
Queue.put(self, item, block=False) | |||
except Full: | |||
# If the queue is full, replace a random item. We need to make sure | |||
# the queue is not emptied was emptied in the meantime, so we lock | |||
# check qsize value. | |||
# the queue was not in the meantime, so we lock and check qsize. |
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.
uh?
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.
undid this change
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
I merged down from |
We have collectors deployed behind an AWS NLB that does TLS termination so we'd like HTTPS support.
This will have conflicts with #1054.