automatic metrics for gRPC client interceptor #917

cnnradams · 2020-07-16T16:00:39Z

part 2/2 of #878

codeboten

Thanks! This is looking pretty good, just a couple of questions in the comments and waiting for an update of the changelog before approving.

codeboten · 2020-07-17T21:51:32Z

ext/opentelemetry-ext-grpc/src/opentelemetry/ext/grpc/__init__.py

@@ -21,7 +21,7 @@
 from opentelemetry.ext.grpc.version import __version__


-def client_interceptor(tracer_provider=None):
+def client_interceptor(tracer_provider=None, meter=None):


curious what the reason for passing in a meter and not a meter_provider here

if we pass a meter_provider, we would need to create/get an exporter and a controller as well. Passing meter is the only way (as far as i can tell) to get everything needed for metrics in 1 argument

I'm ok with either, not sure if @lzchen has a strong preference one way or another here.

Is the purpose of this just to collect the related metrics? Are we leaving it up to the user to choose what to do with it, or is there only one exporter in which you want to use? If it is the former, then using a meter instance would work.

codeboten · 2020-07-17T21:58:59Z

ext/opentelemetry-ext-grpc/src/opentelemetry/ext/grpc/_client.py

        self._tracer = tracer
+        self._meter = meter
+        self._metrics_recorder = TimedMetricRecorder(self._meter, "client")


what happens here if the meter passed in is None? Is the TimedMetricRecorder treated as a no-op?

codeboten · 2020-07-17T22:03:45Z

ext/opentelemetry-ext-grpc/src/opentelemetry/ext/grpc/_client.py

+                    )
+                    raise
+
+                ret = self._trace_result(guarded_span, rpc_info, result)


any reason for this return variable? it doesn't appear to be used anywhere other than in the return

_trace_result assigned rpc_info.response, which is why I did it this way, but it does look pretty bad. Moved the byte count to inside _trace_result and got rid of this variable

codeboten

LGTM

ext/opentelemetry-ext-grpc/src/opentelemetry/ext/grpc/__init__.py

lzchen · 2020-07-21T03:15:26Z

ext/opentelemetry-ext-grpc/src/opentelemetry/ext/grpc/__init__.py

@@ -21,7 +21,7 @@
 from opentelemetry.ext.grpc.version import __version__


-def client_interceptor(tracer_provider=None):
+def client_interceptor(tracer_provider=None, meter=None):


How is client_interceptor actually used? Is this instantiated by the user? How does this relate to [#788]?

interceptor = client_interceptor(meter=meter) self.channel = intercept_channel( grpc.insecure_channel("localhost:25565"), interceptor )

I will add a meter parameter to _instrument if #788 gets merged first (although.. passing a meter makes uninstrumentation difficult. maybe exporter + interval as params is actually a better choice in that case?

An exporter + interval would make more. We can instantiate a specific meter for grpc metrics under the hood (unique name) and start the pipeline implicitly if an exporter is passed in.

switched to using exporter + interval which will do for now, but I still think there needs to be a proper solution to this that doesn't require passing two things to every single instrumentation

lzchen · 2020-07-21T03:25:39Z

ext/opentelemetry-ext-grpc/src/opentelemetry/ext/grpc/__init__.py

@@ -21,7 +21,7 @@
 from opentelemetry.ext.grpc.version import __version__


-def client_interceptor(tracer_provider=None):
+def client_interceptor(tracer_provider=None, meter=None):


How will the metrics be exposed if the instrumentor is auto-instrumented? There's no way to pass in/access a meter if the instrumentor is instantiated through an entry point.

I don't think there is a good way to have the metrics be created on auto instrumentation, since there is no global meterprovider + interval + exporter we can attach to. The meter has to be passed in somehow, and if auto-instrumentation can't pass anything in, then we can't add metrics.

Right. In the above suggestion for export + interval, this only works on explicit instantiation. Seems like we can either expose an API for the user to manually indicate to start exporting metrics, or we can use a default exporter + export interval. Both methods are kind of not great IMO, it either requires the user to know about the implementation/existence of metrics in the first place (and they have to manually call start_pipeline) or it forces them to use an exporter/export interval that is not configurable. Might be something to bring up during the SIG.

lzchen

Does this PR need to be refactored since [https://github.com//pull/788] was merged?

cnnradams · 2020-07-27T14:14:30Z

Does this PR need to be refactored since [https://github.com//pull/788] was merged?

yes, updated it

lzchen · 2020-07-27T16:40:24Z

ext/opentelemetry-ext-grpc/src/opentelemetry/ext/grpc/_client.py

        self._tracer = tracer
+        self._meter = metrics.get_meter(__name__)


If the user is not using the default SDK, DefaultMeter will be used (or some other Meter implementation) which might not have the collect method defined, which will cause this to crash.

Not sure what you mean here? I took this part from ext/system_metrics, is that wrong too?

I'm not sure about ext/system_metrics, but if you are not explicitly using the meter implementation in opentelemetry-sdk, then this would crash, since every tick() in PushController calls meter.collect() which only exists in the sdk implementation.

Also on that note, opentelemetry-sdk would need to be a dependency now correct in setup,cfg correct?

So I should create the meterprovider from opentelemetry-sdk explicitly here even though that means the user can't set it?

I think you can simply just take a dependency on the sdk and include in the examples that you must set meterprovider. Also, since users can autoinstrument the grpc instrumentor, you must not have this logic execute (since meterprovider won't be set).

ext/opentelemetry-ext-grpc/src/opentelemetry/ext/grpc/_client.py

cnnradams requested a review from a team July 16, 2020 16:00

codeboten suggested changes Jul 17, 2020

View reviewed changes

cnnradams force-pushed the grpc_metrics_tests branch 2 times, most recently from d8d2b10 to 105ec0d Compare July 20, 2020 15:11

codeboten approved these changes Jul 20, 2020

View reviewed changes

lzchen reviewed Jul 21, 2020

View reviewed changes

ext/opentelemetry-ext-grpc/src/opentelemetry/ext/grpc/__init__.py Show resolved Hide resolved

lzchen reviewed Jul 21, 2020

View reviewed changes

cnnradams force-pushed the grpc_metrics_tests branch from 964aff1 to 20e0a2b Compare July 21, 2020 16:26

lzchen requested changes Jul 24, 2020

View reviewed changes

cnnradams force-pushed the grpc_metrics_tests branch from bc85b1f to 17633bb Compare July 27, 2020 14:14

cnnradams force-pushed the grpc_metrics_tests branch 2 times, most recently from bc375f3 to bdb5225 Compare July 27, 2020 15:01

lzchen reviewed Jul 27, 2020

View reviewed changes

ext/opentelemetry-ext-grpc/src/opentelemetry/ext/grpc/_client.py Outdated Show resolved Hide resolved

lzchen reviewed Jul 27, 2020

View reviewed changes

ext/opentelemetry-ext-grpc/src/opentelemetry/ext/grpc/_client.py Outdated Show resolved Hide resolved

cnnradams force-pushed the grpc_metrics_tests branch from c56f5db to bc5363b Compare July 27, 2020 21:01

lzchen approved these changes Jul 29, 2020

View reviewed changes

cnnradams added 7 commits July 29, 2020 15:00

grpc client metrics

84a5219

Fixes, changelog

ae904fa

use exporter + interval instead of meter

1ba6a17

update to auto grpc instrumentor

47881bb

check for exporter pass

76f4c78

check interval too

c083aa4

Use SDK MeterProvider

7278b51

cnnradams force-pushed the grpc_metrics_tests branch from c140121 to 7278b51 Compare July 29, 2020 19:01

lzchen merged commit fc58032 into open-telemetry:master Jul 29, 2020

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020

Updating out of date nyc packages (open-telemetry#917)

0d1c82c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

automatic metrics for gRPC client interceptor #917

automatic metrics for gRPC client interceptor #917

cnnradams commented Jul 16, 2020

codeboten left a comment

codeboten Jul 17, 2020

cnnradams Jul 20, 2020 •

edited

Loading

codeboten Jul 20, 2020

lzchen Jul 21, 2020 •

edited

Loading

codeboten Jul 17, 2020

cnnradams Jul 20, 2020

codeboten Jul 17, 2020

cnnradams Jul 20, 2020

codeboten left a comment

lzchen Jul 21, 2020

cnnradams Jul 21, 2020

lzchen Jul 21, 2020

cnnradams Jul 21, 2020

lzchen Jul 21, 2020

cnnradams Jul 21, 2020

lzchen Jul 21, 2020

lzchen left a comment

cnnradams commented Jul 27, 2020

lzchen Jul 27, 2020

cnnradams Jul 27, 2020

lzchen Jul 27, 2020

lzchen Jul 27, 2020

cnnradams Jul 28, 2020

lzchen Jul 28, 2020

		self._tracer = tracer
		self._meter = metrics.get_meter(__name__)

automatic metrics for gRPC client interceptor #917

automatic metrics for gRPC client interceptor #917

Conversation

cnnradams commented Jul 16, 2020

codeboten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnnradams Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzchen Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeboten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzchen left a comment

Choose a reason for hiding this comment

cnnradams commented Jul 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnnradams Jul 20, 2020 •

edited

Loading

lzchen Jul 21, 2020 •

edited

Loading