Add ocagent stats exporter.

[prateekr]

No description provided.

[songy23]

You can now use opencensus-proto from Pypi (https://pypi.org/project/opencensus-proto/) instead of vendoring the proto-generated files. See #596 for an example.

[prateekr]

Ah I was worried it wouldn't be ready at this point; will go ahead and update.

[reyang]

Please also update the README and CHANGELOG, thanks!

[reyang]

Overall looks good, I've left some comments. Thanks.

[reyang]

LGTM. Thanks!

[prateekr]

One of the tests globally modifies channels created through grpc.insecure to always attach OpenCensusClientInterceptor. This is causing test_stats_exporter to pull in this interceptor as well.

The grpc.StreamStreamClientInterceptor abstract class states stream interceptor method should return an object that's both a call (implementing the response iterator) and a future.
https://github.com/grpc/grpc/blob/8054a731d1486e439e6becb1987b1e97246e6476/src/python/grpcio/grpc/__init__.py#L529-L561

However, OpenCensusClientInterceptor just returns a python generator, thus not respecting the defined interface. This in turn breaks all users (including api_core utils) that rely on Future and Call functionality.

opencensus-python/contrib/opencensus-ext-grpc/opencensus/ext/grpc/client_interceptor.py

Lines 228 to 238 in c59a087

	response_it = continuation(
	new_details,
	new_request_iterator)
	response_it = grpc_utils.wrap_iter_with_message_events(
	request_or_response_iter=response_it,
	span=current_span,
	message_event_type=time_event.Type.RECEIVED
	)
	response_it = grpc_utils.wrap_iter_with_end_span(response_it)
	return response_it

I've created a wrapper iterator that I now return here to ensure the current use cases work. In a followup, it'd be good to implement the current the NotImplemented methods in their entirety.
@reyang Could you take another pass at these changes?

[c24t]

Some minor style comments, but otherwise the changes look great. Thanks for adding this @prateekr.

The grpc.StreamStreamClientInterceptor abstract class states stream interceptor method should return an object that's both a call (implementing the response iterator) and a future.

Thanks for adding the class. Can you add a comment here describing why we need it? It is not at all obvious looking at the (opencensus) source.

What are the odds that this integration is going to break with an update to grpc seeing as this is an experimental API?

One of the tests globally modifies channels created through grpc.insecure to always attach OpenCensusClientInterceptor

Even if your class fixes test_stats_exporter it sounds like we need to stop the other test from patching this globally!

[c24t]

FYI the coverage check is failing for WrappedResponseIterator:

contrib/opencensus-ext-grpc/opencensus/ext/grpc/utils.py
75% 45-47, 93, 96, 99, 102, 105, 108, 111, 114, 117, 120, 126, 129, 132

contrib/opencensus-ext-ocagent/opencensus/ext/ocagent/stats_exporter/__init__.py
98% 91, 79->91

I don't see any problem with adding # pragma: NO COVER to the intentionally unimplemented methods.

[prateekr]

What are the odds that this integration is going to break with an update to grpc seeing as this is an experimental API?

Not sure, but the grpc interceptor has been around for over two years now, so I imagine it's maturing. Also, I don't think the rpc stream return object class is experimental, so any potential changes will likely continue to return this.

gRPC api breakages should be a lot easier to catch with the tests added in this PR for the grpc extension.

[reyang]

@prateekr sorry for my delayed response. The code looks good to me.

I have a general question @c24t, do we want to have a special context variable for exporters, so we don't intercept any operation (besides calculating some exporter specific metrics) while running exporter logic.

[prateekr]

Even if your class fixes test_stats_exporter it sounds like we need to stop the other test from patching this globally!

Agreed, looks like contrib/opencensus-ext-google-cloud-clientlibs/tests/test_google_cloud_clientlibs_trace.py is an offender. I've patched it enough so the tests in this PR pass. Leaving a wholistic cleanup of this test to a future CL.

[c24t]

do we want to have a special context variable for exporters, so we don't intercept any operation (besides calculating some exporter specific metrics) while running exporter logic

So you imagine checking for a context var in the integrations and -- if it's set -- passing calls through to the wrapped library (e.g. grpcio, requests) instead of intercepting them to attach trace data/collect metrics?

If it's possible I'd prefer not to monkey patch libraries like this at all, but I understand one of the reasons to do this (and why other projects/libraries do bytecode manipulation) is to instrument e.g. grpc calls even when they're buried inside another library. I don't have any suggestions to address this that aren't colossal hacks.

At least in this specific case the problem is that a test isn't cleaning up after itself. Even if the integration acted as a no-op during the test it would still be a problem that the integration was loaded at all.

[c24t]

@prateekr once coverage is fixed this looks good to merge.

[reyang]

So you imagine checking for a context var in the integrations and -- if it's set -- passing calls through to the wrapped library (e.g. grpcio, requests) instead of intercepting them to attach trace data/collect metrics?

Yes, at least we don't want to collect traces. We might need to collect metrics in some cases though.
Today we don't have a contract, so exporters ended up doing hacks, imagine we have something MySQL exporter or PostgreSQL exporter, we will need to invent another hack. It'll be much cleaner if we have a flag to indicate whether the code is running inside an exporter context, so integrations and exporters can be decoupled through that contract. (This is similar to where we have a meta logger)

If it's possible I'd prefer not to monkey patch libraries like this at all, but I understand one of the reasons to do this (and why other projects/libraries do bytecode manipulation) is to instrument e.g. grpc calls even when they're buried inside another library. I don't have any suggestions to address this that aren't colossal hacks.

Me too! I think the long term direction is encourage 3rd party framework/libraries adding these probes, rather than having OpenCensus reverse engineering and hacking in the probes.
It will take time as you said, for now we will need to rely on community contribution to maintain such a big matrix of hacks.

At least in this specific case the problem is that a test isn't cleaning up after itself. Even if the integration acted as a no-op during the test it would still be a problem that the integration was loaded at all.

Yup, this question is not specific to this PR. @prateekr's approach looks good to me within the PR context, and I've signed off.

[c24t]

It'll be much cleaner if we have a flag to indicate whether the code is running inside an exporter context, so integrations and exporters can be decoupled through that contract.

I think that's a good idea, and if we do implement a general purpose kill-switch for integrations it's probably something that we want in more than just the python client.