Meter provider api #39
Conversation
dnarula-mw
commented
Oct 11, 2023
dnarula-mw
commented
- Creates the MeterProvider API class with methods "getMeter" and "setMeterProvider"
- Creates the MeterProviderProxy for the API
- Add a unit test for get and set meter provider
api/metrics/include/opentelemetry-matlab/metrics/MeterProviderProxy.h
Outdated
Show resolved
Hide resolved
| public: | ||
| MeterProviderProxy(nostd::shared_ptr<metrics_api::MeterProvider> mp) : CppMeterProvider(mp) { | ||
| REGISTER_METHOD(MeterProviderProxy, getMeter); | ||
| MeterProviderProxy(nostd::shared_ptr<metrics_api::MeterProvider> mp) : libmexclass::opentelemetry::MeterProviderProxy(mp), CppMeterProvider(mp) { |
There was a problem hiding this comment.
Calling the base class constructor and assigning directly to CppMeterProvider is redundant. The base class constructor already assigns to CppMeterProvider. I think you can leave out CppMeterProvider(mp) here on this line
There was a problem hiding this comment.
Originally I left out the CppMeterProvider(mp) from this line, but after merging with Rick's code it caused a crash. I had to add it back in or else the "addMetricReader" would cause a crash. Should I find another way to fix that crash instead?
There was a problem hiding this comment.
If it is causing a crash, we need to understand why. CppMeterProvider(mp) here should be completely redundant. I think we should remove it, find out if it is really crashing, and if so, determine why.
There was a problem hiding this comment.
Ok I will look into this, thanks.
test/tmetrics.m
Outdated
| function testGetSetMeterProvider(testCase) | ||
| % testGetSetMeterProvider: setting and getting global instance of MeterProvider | ||
| exporter = opentelemetry.exporters.otlp.OtlpHttpMetricExporter(... | ||
| "PreferredAggregationTemporality", "Delta"); |
There was a problem hiding this comment.
I don't think you need "Delta" temporality here. I would just call the exporter constructor with no input and use the default temporality.
There was a problem hiding this comment.
For some reason after implementing all these changes and running the tests, all the tracer tests which are run after the metrics tests fail. I added the Delta temporality back in, and the tracer tests are now passing again. Do you know why that could be happening?
There was a problem hiding this comment.
How did the tests fail? What are the error messages?
There was a problem hiding this comment.
The resourceSpan error is unrelated. I think currently it is a sporadic error depending on the run. The reason why it is happening is that even when we do "clear mp" before readJsonResults, we are still not stopping the MeterProvider from regularly exporting. The MeterProvider mp contains a shared pointer that points to the cpp MeterProvider object. But that cpp MeterProvider object is also pointed to by the global MeterProvider with a shared pointer. As long as there is at least one shared pointer pointing to it, the cpp MeterProvider is not getting destroyed.
After the metric tests are completed and when the tracing tests are running, the collector is getting data from both metrics and tracing. Depending on the timing, the tracing tests sometimes pick up the metric results, and the metric results doesn't have the resourceSpan field.
So we need to figure out how to deal with this larger issue. But that can be after your PR gets accepted. I would recommend you can continue to remove the "delta" temporality. If the tests fail, we can ignore for now, and deal with it after.
There was a problem hiding this comment.
Ok sounds good thanks. I deleted my earlier comment about the error with tcontextPropagation because that was just caused by a typo so that's fine.
I just confirmed by testing a few times that the resourceSpan error does seem to occur when the "delta" temporality is removed, and the error goes away when I add it back. I'm not sure what the connection is there, but I can still remove the "delta" temporality if you'd like.
