Skip to content

tracer: support lightstep#1483

Merged
jazzdan merged 2 commits into
masterfrom
dmiller/ch2259/add-support-for-lightstep
Apr 12, 2019
Merged

tracer: support lightstep#1483
jazzdan merged 2 commits into
masterfrom
dmiller/ch2259/add-support-for-lightstep

Conversation

@jazzdan

@jazzdan jazzdan commented Apr 11, 2019

Copy link
Copy Markdown
Contributor

We might need to support more fields in the lightStep.Options struct before I merge this in but I wanted to validate the approach.

@jazzdan
jazzdan requested review from landism and nicks April 11, 2019 22:23
@landism

landism commented Apr 12, 2019

Copy link
Copy Markdown
Member

What's there looks fine. The two main questions that come to mind are:

What's it going to look like with more lightstep options? A doc with a half dozen environment variables?

Is synclet configuration explicitly not worth worrying about for now? This leaves us in the mildly weird position of having synclet traces with no parent tilt traces, but I don't know how likely we are to ever do anything with the traces anyway.

@jazzdan

jazzdan commented Apr 12, 2019

Copy link
Copy Markdown
Contributor Author

@landism

What's it going to look like with more lightstep options? A doc with a half dozen environment variables?

I think if it came to that we would re-evaluate. This is just a short term solution to unblock some folks. In the long term I think all of this config should live in the Tiltfile and it would be especially dope if Tilt could react to tracing being enabled/disabled without having to be restarted.

Is synclet configuration explicitly not worth worrying about for now? This leaves us in the mildly weird position of having synclet traces with no parent tilt traces, but I don't know how likely we are to ever do anything with the traces anyway.

Yeah it's pretty underutilized (though we should verify that). The folks that are asking for tracing don't use it, either.

@jazzdan
jazzdan merged commit 495418e into master Apr 12, 2019
@nicks
nicks deleted the dmiller/ch2259/add-support-for-lightstep branch March 31, 2021 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants