Cloud Trace exporter

[AndrewAXue]

Addresses #287.
Adding a Cloud Trace (formerly known as stackdriver) trace exporter.

[sjbrunst]

Quick initial pass on cloud_trace/init.py

[kintel]

Quick notes from offline discussion:

• This supersedes Add stackdriver trace exporter #288
• Chris suggested we merge this into this repo for now, just to get some eyeballs on it, and tests going
• ASAP after merging, move it to an external repo (Move Cloud Trace (Stackdriver) exporter #712)

[c24t]

One blocking comment about Span-valued span parents, some questions about client types, and some non-blocking nits.

I'll have to take another pass at the examples, but this looks like a great start otherwise. Thanks for picking this up @AndrewAXue!

[owais]

Not specific to this PR but should SpanExportResult be able to encapsulate other error information like the original exception? If all exporters try to catch and swallow every exception globally and then return just one value to represent failure, we might be losing out on valuable information. Perhaps processor/exporter should be able to return the original python exceptions or error messages+stack traces along with a failure status. This information can be used by the caller to show helpful error messages to users by utilizing a single global error handler instead of relying on every processor to do the right thing by using the correct logger. In general, I feel error handling should propagate back up and be taken care of preferably in a single place. Especially if we are catching all possible exceptions.

[AndrewAXue]

I agree with you, but I would prefer to handle that in a seperate PR :)

[owais]

For sure, I'm not suggesting you change it in this PR but wanted to bring this up for the Python group in general.

[toumorokoshi]

I agree, although in this case hopefully the logger.warning helps.

[kintel]

One structural question: The majority of the code lives in init.py. This strikes me as an odd choice. Is there a reason for this, e.g. is there a style guide that prefers using init.py in certain cases?

[kintel]

Before merging, we should create a minimal example, which can be manually run to sanity-check the exporter. I quickly tried to shoehorn this into an existing program, but got this error:

Error while writing to Cloud Trace: list indices must be integers or slices, not str
Error 'TruncatableString' object is not iterable when creating span

[kintel]

Oh, and for any issues, comments or suggestions that we decide not to address right now, please open an issue in https://github.com/GoogleCloudPlatform/opentelemetry-operations-python, and link it here.

[AndrewAXue]

Before merging, we should create a minimal example, which can be manually run to sanity-check the exporter. I quickly tried to shoehorn this into an existing program, but got this error:

Error while writing to Cloud Trace: list indices must be integers or slices, not str
Error 'TruncatableString' object is not iterable when creating span

Hey Marius, there are examples in opentelemetry-ext-cloud-trace/examples. One of which is just a basic trace, the 2nd and 3rd are example server + clients. Can you send me the exact code you tried to use this exporter in?

[kintel]

Taking the credentials/test environment discussion offline for debugging (I see similar issues with the examples in my env).

[kintel]

Hi Andrew, I did a relatively thorough review, focusing mostly on the Trace API interaction.
Left a handful of comments.
As usual, feel free to defer comments to a later PRs upon creating appropriate GitHub issues.

[toumorokoshi]

Thanks for the PR! There's a few blocking comments for me. The major sticking point to me is the modification of a vendor-agnostic instrumentation specifically for cloud trace. Maybe we can discuss further here or through gitter on a solution there.

[c24t]

@AndrewAXue FYI force pushing breaks github's inline code comments (among other problems). Since we're squash-merging PRs anyway there's usually not much reason to force-push, and it makes reviewers' lives easier if you don't.

[c24t]

A few questions, but no more blocking comments from me. Thanks @AndrewAXue!

[c24t]

@toumorokoshi if this LGTY it should be ready to merge.

[toumorokoshi]

Great, thanks!

[toumorokoshi]

this chunk of code (along with the warning) could be refactored into _extract_attributes.

I see some boilerplate here and extract_events that includes the max check then logs a warning. This feels intuitive to me to put the warning around some attributes will be dropped into the _extract_attributes itself, or maybe even the BoundedDict itself.

[AndrewAXue]

Hey Yusuke, so I also noticed that the max length check + warning is repeated in extract_events and extract_links. The reason I kept it that way is so I can print more informative warnings then just "Too many attributes, dropped some", doing it this way I can say which event or link had too many attributes.

[toumorokoshi]

sounds good, although it feels like that's still achievable after a refactor.