Skip to content
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

Add Stackdriver export #6217

Closed
wants to merge 5 commits into from
Closed

Conversation

anacrolix
Copy link
Contributor

I'm using this to export traces to Stackdriver. I'd like some feedback. Perhaps the exporter Location and default sample rate can be taken from the environment, and the export only attempted if particular environment variables are set.

@magik6k magik6k self-requested a review April 15, 2019 10:48
@magik6k
Copy link
Member

magik6k commented Apr 18, 2019

Are there any discussions / issues linked to this?

@anacrolix
Copy link
Contributor Author

@magik6k What sort are you referring to? There's efforts in filecoin to do tracing, and libp2p to tracing and metrics via OpenCensus.

@raulk
Copy link
Member

raulk commented Apr 22, 2019

@anacrolix – I believe @magik6k is referring to the attempt to introduce Stackdriver specifically. It's not clear to me the kind of feedback you are seeking, either.

(It's obvious this approach will not be accepted as-is -- it's tied to your personal Google account for starters).

@anacrolix
Copy link
Contributor Author

I'd like to know if OC exporters could be supported, via environment flags, similar to as is done in this PR for Stackdriver.

@anacrolix
Copy link
Contributor Author

I'll update this PR to be more general, to reflect how I've done this in other projects.

@magik6k
Copy link
Member

magik6k commented Apr 23, 2019

Using env vars feels better.

What is the difference in binary size with this patch applied? Can this be made into a plugin?

@anacrolix
Copy link
Contributor Author

@magik6k

master              37757316
stackdriver-tracing 43943460

@anacrolix
Copy link
Contributor Author

As a plugin, I wonder if having upstream projects in libp2p directly reference opencensus tracing will defeat the purpose (the dependencies will be pulled in anyway). In particular, while there is no OC tracing in libp2p yet, I think there will be soon, and the cost will already be paid to add that there. It may be possible to avoid that cost, by tapping into OC's runtime/trace support, but then we lose a lot of extra features there, and there'll need to be a lot of fiddly OC injection outside libp2p to regain the extra OC tracing features by consumers. I.e. everything that wants OC tracing enabled, will need to call out to the plugin before calling into anything related to libp2p.

@magik6k
Copy link
Member

magik6k commented Apr 24, 2019

6MB is a lot, especially given that it isn't going to be used by 99.99% of users. Can we hide this behind a build tag, say tracing?

@Stebalien
Copy link
Member

It looks like a almost all of the weight comes from the stackdriver exporter, not opencensus. Putting the stackdriver code in a plugin should help quite a bit.

@anacrolix
Copy link
Contributor Author

anacrolix commented Apr 25, 2019

Thanks @Stebalien, that seems like the best solution.

@Stebalien
Copy link
Member

(we'll reopen this when we pick this work back up)

@Stebalien Stebalien closed this May 15, 2019
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.

4 participants