Lightstep-Access-Token Header Addition by cboppert · Pull Request #64 · lightstep/lightstep-tracer-python

cboppert · 2019-04-19T19:12:31Z

This PR adds the Lightstep-Access-Token header to the lightstep tracer python.

It also includes some readme updates I discovered while setting up the local python tracer example.

cboppert · 2019-04-19T19:13:23Z

@@ -1 +1 @@
-4.0.3
+4.0.4


Not certain if this version is updated manually or done automatically during deploy

JulianGriggs · 2019-04-19T21:10:37Z

Structurally, I think it would be best to split the README updates and the code updates into separate PRs.
It looks like this tracer supports HTTP and Thrift. Have you investigated adding the header to the Thrift transport?

cboppert · 2019-04-19T21:30:00Z

@JulianGriggs I've tested with both use_http=True and use_thrift=True. I looked through the ThriftConnection and didn't see headers to add so it must be through the HTTP one somehowe.

cboppert · 2019-04-19T21:30:48Z

I'll split it up

JulianGriggs · 2019-04-20T00:37:16Z

@@ -1,3 +1,7 @@
+<a name="4.0.4"></a>
+## [4.0.2](https://github.com/lightstep/lightstep-tracer-python/compare/4.0.2...4.0.3)
+* Add LightStep-Access-Token headers to outgoing requests.


nit: header should be singular

JulianGriggs · 2019-04-22T20:34:14Z

+```python
+return lightstep.Tracer(
+    component_name='WhatIDoWhatIDidnt',
+    access_token='587145376229b4e8cc4d99d948bb967b',


Delete this access token and the collector host. This shouldn't be committed. In fact, I think to demonstrate this point you should do

return lightstep.Tracer( ...., use_http=False, use_thrift=True, ...., )

Woops, sorry about that

JulianGriggs · 2019-04-22T20:37:12Z

@cboppert Based upon the previous commit in this repo 98b87bc, I think that version bumps are typically done separately from code updates. As such, I think we should continue doing it that way

cboppert · 2019-04-22T20:43:32Z

Alright

JulianGriggs · 2019-04-22T20:56:54Z

@@ -1 +1 @@
-4.0.3
+4.0.3


nit: can you add back the newline so that there is no diff here

JulianGriggs · 2019-04-22T20:59:25Z

+as enabling use_thrift.
+
+```python
+return lightstep.Tracer(


I still think that detailing the other options to this call makes it more difficult to consume the textual information. As a reader, I start wondering if the other parameters are relevant.

Okay, can delete.

JulianGriggs · 2019-04-22T21:00:17Z

 ```

+### Thrift
+When using apache thrift rpc, make sure to both disable use_http by setting it to False as well


Can we wrap use_http, use_thrift, False, and True with ticks (`) so that they are more obviously read as code parameters

JulianGriggs · 2019-04-22T21:01:08Z

@cboppert One last comment, if you could squash these commits together that would be great -- to clean it up a little bit.

cboppert · 2019-04-22T21:03:14Z

@JulianGriggs Wouldn't we just squash on merge?

austinlparker

Overall it's fine other than the readme.

austinlparker · 2019-04-23T14:53:49Z

 ```

+### Thrift
+When using apache thrift rpc, make sure to both disable use_http by setting it to False as well


You don't need to explicitly disable the transport you don't want (see

lightstep-tracer-python/lightstep/recorder.py

Line 59 in 8422866

if use_http:

)

Hey @austinlparker use_http is set to True by default as you can see in

lightstep-tracer-python/lightstep/recorder.py

Line 48 in 8422866

use_http=True,

So if you don't set use_http to False and only set use_thrift to True, then it always runs the first if block and never hits the second.

That strikes me as a bug then, we should fix that.

I agree, but it's out of scope for this

I'll just fix this in a separate PR.

austinlparker · 2019-04-23T14:56:19Z

+When using apache thrift rpc, make sure to both disable use_http by setting it to False as well
+as enabling use_thrift.
+
+```python


For consistency, we generally prefer the options block in README code samples to be minimal. If you want to improve the README by adding a table that explains the options, that would be good. See https://github.com/lightstep/lightstep-tracer-csharp/blob/master/README.md#advanced-usage for an example.

I'll address this in a separate PR

cboppert added 4 commits April 18, 2019 17:07

Add access token heade

18d9f96

Some README updates

6a8eea0

Update minor minor version

842f884

Update minor version across repo

ab6c3dd

cboppert commented Apr 19, 2019

View reviewed changes

cboppert added 2 commits April 19, 2019 15:14

Canonicalize LightStep-Access-Token to Lightstep-Access-Token

976be3f

Add access token from auth not report

6f04546

cboppert requested review from JulianGriggs, austinlparker and frenchfrywpepper April 19, 2019 20:47

cboppert closed this Apr 19, 2019

cboppert reopened this Apr 19, 2019

Move readme updates to its own PR

fd71f95

JulianGriggs reviewed Apr 20, 2019

View reviewed changes

cboppert added 3 commits April 22, 2019 11:14

Update changelog to say header instead of headers

a11cfc6

Add Lightstep-Access-Token header to python thrift connections

1535c8c

Update README to mention thrift options

3806a4c

JulianGriggs reviewed Apr 22, 2019

View reviewed changes

cboppert closed this Apr 22, 2019

cboppert reopened this Apr 22, 2019

cboppert added 2 commits April 22, 2019 16:45

Undo mess up where committed token to version control

1c0d83d

Do version update in separate pr

4fcab8b

cboppert requested review from JulianGriggs and carlosalberto April 22, 2019 20:56

JulianGriggs reviewed Apr 22, 2019

View reviewed changes

cboppert mentioned this pull request Apr 22, 2019

Access Token Header Version Update (4.0.4) #65

Closed

JulianGriggs reviewed Apr 22, 2019

View reviewed changes

austinlparker suggested changes Apr 23, 2019

View reviewed changes

austinlparker approved these changes Apr 23, 2019

View reviewed changes

cboppert merged commit 6f9007d into master Apr 23, 2019

carlosalberto deleted the cody/access-token-headers branch May 31, 2019 16:32

		@@ -1 +1 @@
		4.0.3 No newline at end of file
		4.0.4

		@@ -1 +1 @@
		4.0.3 No newline at end of file
		4.0.3

Conversation

cboppert commented Apr 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JulianGriggs commented Apr 19, 2019

Uh oh!

cboppert commented Apr 19, 2019

Uh oh!

cboppert commented Apr 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JulianGriggs commented Apr 22, 2019

Uh oh!

cboppert commented Apr 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JulianGriggs commented Apr 22, 2019

Uh oh!

cboppert commented Apr 22, 2019

Uh oh!

austinlparker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants