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.

Sign up for GitHub

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

Meter provider api #39

Merged
merged 11 commits into from
Oct 12, 2023
Merged

Meter provider api #39

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
40ecc12
Creating MeterProvider API
dnarula-mw Oct 5, 2023
1773af8
Finishing and cleaning up MeterProvider api
dnarula-mw Oct 6, 2023
ac793c5
Adding test for get and set meter
dnarula-mw Oct 11, 2023
a0777dd
Resolving merge conflicts
dnarula-mw Oct 11, 2023
ec00310
Fixing error from merge
dnarula-mw Oct 11, 2023
3af790a
Removing .vs folder
dnarula-mw Oct 11, 2023
8268a15
Lowering reader interval for test
dnarula-mw Oct 12, 2023
f9b7c45
clearning meter provider in test
dnarula-mw Oct 12, 2023
39e01fa
fixing typo
dnarula-mw Oct 12, 2023
a630aa7
Addressing PR comments
dnarula-mw Oct 12, 2023
fd75583
Fixing crash and comment out test
dnarula-mw Oct 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ set(OPENTELEMETRY_PROXY_SOURCES
${TRACE_API_SOURCE_DIR}/SpanProxy.cpp
${TRACE_API_SOURCE_DIR}/SpanContextProxy.cpp
${COMMON_API_SOURCE_DIR}/attribute.cpp
${METRICS_API_SOURCE_DIR}/MeterProviderProxy.cpp
${METRICS_API_SOURCE_DIR}/MeterProxy.cpp
${METRICS_API_SOURCE_DIR}/CounterProxy.cpp
${METRICS_API_SOURCE_DIR}/UpDownCounterProxy.cpp
Expand Down
1 change: 1 addition & 0 deletions OtelMatlabProxyFactory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ libmexclass::proxy::MakeResult
OtelMatlabProxyFactory::make_proxy(const libmexclass::proxy::ClassName& class_name,
const libmexclass::proxy::FunctionArguments& constructor_arguments) {

REGISTER_PROXY(libmexclass.opentelemetry.MeterProviderProxy, libmexclass::opentelemetry::MeterProviderProxy);
REGISTER_PROXY(libmexclass.opentelemetry.TracerProviderProxy, libmexclass::opentelemetry::TracerProviderProxy);
//REGISTER_PROXY(libmexclass.opentelemetry.TracerProxy, libmexclass::opentelemetry::TracerProxy);
REGISTER_PROXY(libmexclass.opentelemetry.SpanProxy, libmexclass::opentelemetry::SpanProxy);
Expand Down
2 changes: 1 addition & 1 deletion api/metrics/+opentelemetry/+metrics/Meter.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
Proxy % Proxy object to interface C++ code
end

methods (Access={?opentelemetry.sdk.metrics.MeterProvider})
methods (Access={?opentelemetry.sdk.metrics.MeterProvider, ?opentelemetry.metrics.MeterProvider})

function obj = Meter(proxy, mtname, mtversion, mtschema)
% Private constructor. Use getMeter method of MeterProvider
Expand Down
62 changes: 62 additions & 0 deletions api/metrics/+opentelemetry/+metrics/MeterProvider.m
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
classdef MeterProvider < handle
% A meter provider stores a set of configurations used in a distributed
% metrics system.

% Copyright 2023 The MathWorks, Inc.

properties (Access={?opentelemetry.sdk.metrics.MeterProvider})
Proxy % Proxy object to interface C++ code
end

methods (Access={?opentelemetry.metrics.Provider, ?opentelemetry.sdk.metrics.MeterProvider})
function obj = MeterProvider(skip)
% constructor
% "skip" input signals skipping construction
if nargin < 1 || skip ~= "skip"
obj.Proxy = libmexclass.proxy.Proxy("Name", ...
"libmexclass.opentelemetry.MeterProviderProxy", ...
"ConstructorArguments", {});
end
end
end

methods
function meter = getMeter(obj, mname, mversion, mschema)
% GETMETER Create a meter object used to generate metrics.
% M = GETMETER(MP, NAME) returns a meter with the name
% NAME that uses all the configurations specified in meter
% provider MP.
%
% M = GETMETER(MP, NAME, VERSION, SCHEMA) also specifies
% the meter version and the URL that documents the schema
% of the generated meters.
%
% See also OPENTELEMETRY.METRICS.METER
arguments
obj
mname
mversion = ""
mschema = ""
end
% name, version, schema accepts any types that can convert to a
% string
import opentelemetry.common.mustBeScalarString
mname = mustBeScalarString(mname);
mversion = mustBeScalarString(mversion);
mschema = mustBeScalarString(mschema);
id = obj.Proxy.getMeter(mname, mversion, mschema);
meterproxy = libmexclass.proxy.Proxy("Name", ...
"libmexclass.opentelemetry.MeterProxy", "ID", id);
meter = opentelemetry.metrics.Meter(meterproxy, mname, mversion, mschema);
end

function setMeterProvider(obj)
% SETMETERPROVIDER Set global instance of meter provider
% SETMETERPROVIDER(MP) sets the meter provider MP as
% the global instance.
%
% See also OPENTELEMETRY.METRICS.PROVIDER.GETMETERPROVIDER
obj.Proxy.setMeterProvider();
end
end
end
27 changes: 27 additions & 0 deletions api/metrics/+opentelemetry/+metrics/Provider.m
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
classdef Provider
% Get and set the global instance of meter provider

% Copyright 2023 The MathWorks, Inc.

methods (Static)
function p = getMeterProvider()
% Get the global instance of meter provider
% MP = OPENTELEMETRY.METRICS.PROVIDER.GETMETERPROVIDER gets
% the global instance of meter provider.
%
% See also OPENTELEMETRY.METRICS.PROVIDER.SETMETERPROVIDER

p = opentelemetry.metrics.MeterProvider();
end

function setMeterProvider(p)
% Set the global instance of meter provider
% OPENTELEMETRY.METRICS.PROVIDER.GETMETERPROVIDER(MP) sets
% MP as the global instance of meter provider.
%
% See also OPENTELEMETRY.METRICS.PROVIDER.GETMETERPROVIDER
p.setMeterProvider();
end
end

end
46 changes: 46 additions & 0 deletions api/metrics/include/opentelemetry-matlab/metrics/MeterProviderProxy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// Copyright 2023 The MathWorks, Inc.

#pragma once

#include "libmexclass/proxy/Proxy.h"
#include "libmexclass/proxy/method/Context.h"

#include "opentelemetry/sdk/metrics/meter_provider_factory.h"
#include "opentelemetry/sdk/resource/resource.h"
#include "opentelemetry/exporters/otlp/otlp_http_exporter_factory.h"
#include "opentelemetry/metrics/meter_provider.h"
#include "opentelemetry/metrics/provider.h"
#include "opentelemetry/metrics/noop.h"

namespace metrics_api = opentelemetry::metrics;
namespace metrics_sdk = opentelemetry::sdk::metrics;
namespace metrics_exporter = opentelemetry::exporter::otlp;
namespace nostd = opentelemetry::nostd;
namespace resource = opentelemetry::sdk::resource;
dnarula-mw marked this conversation as resolved.
Show resolved Hide resolved

namespace libmexclass::opentelemetry {
class MeterProviderProxy : public libmexclass::proxy::Proxy {
public:
MeterProviderProxy(nostd::shared_ptr<metrics_api::MeterProvider> mp) : CppMeterProvider(mp) {
REGISTER_METHOD(MeterProviderProxy, getMeter);
REGISTER_METHOD(MeterProviderProxy, setMeterProvider);
}

// Static make method should only be used by getMeterProvider. It gets the global instance
// instead of creating a new instance
static libmexclass::proxy::MakeResult make(const libmexclass::proxy::FunctionArguments& constructor_arguments) {
return std::make_shared<MeterProviderProxy>(metrics_api::Provider::GetMeterProvider());
}

void getMeter(libmexclass::proxy::method::Context& context);

void setMeterProvider(libmexclass::proxy::method::Context& context);

nostd::shared_ptr<metrics_api::MeterProvider> getInstance() {
return CppMeterProvider;
}

protected:
nostd::shared_ptr<metrics_api::MeterProvider> CppMeterProvider;
};
} // namespace libmexclass::opentelemetry
39 changes: 39 additions & 0 deletions api/metrics/src/MeterProviderProxy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// Copyright 2023 The MathWorks, Inc.

#include "opentelemetry-matlab/metrics/MeterProviderProxy.h"
#include "opentelemetry-matlab/metrics/MeterProxy.h"
#include "libmexclass/proxy/ProxyManager.h"

#include "opentelemetry/metrics/provider.h"

#include "MatlabDataArray.hpp"
namespace libmexclass::opentelemetry {
void MeterProviderProxy::getMeter(libmexclass::proxy::method::Context& context) {
// Always assumes 3 inputs
matlab::data::StringArray name_mda = context.inputs[0];
std::string name = static_cast<std::string>(name_mda[0]);
matlab::data::StringArray version_mda = context.inputs[1];
std::string version = static_cast<std::string>(version_mda[0]);
matlab::data::StringArray schema_mda = context.inputs[2];
std::string schema = static_cast<std::string>(schema_mda[0]);

auto mt = CppMeterProvider->GetMeter(name, version, schema);

// instantiate a MeterProxy instance
MeterProxy* newproxy = new MeterProxy(mt);
auto mtproxy = std::shared_ptr<libmexclass::proxy::Proxy>(newproxy);

// obtain a proxy ID
libmexclass::proxy::ID proxyid = libmexclass::proxy::ProxyManager::manageProxy(mtproxy);

// return the ID
matlab::data::ArrayFactory factory;
auto proxyid_mda = factory.createScalar<libmexclass::proxy::ID>(proxyid);
context.outputs[0] = proxyid_mda;
}

void MeterProviderProxy::setMeterProvider(libmexclass::proxy::method::Context& context) {
metrics_api::Provider::SetMeterProvider(CppMeterProvider);
}

} // namespace libmexclass::opentelemetry
26 changes: 3 additions & 23 deletions sdk/metrics/+opentelemetry/+sdk/+metrics/MeterProvider.m
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
classdef MeterProvider < handle
classdef MeterProvider < opentelemetry.metrics.MeterProvider & handle
% An SDK implementation of meter provider, which stores a set of configurations used
% in a metrics system.

% Copyright 2023 The MathWorks, Inc.

properties (Access=private)
Proxy
isShutdown (1,1) logical = false
dnarula-mw marked this conversation as resolved.
Show resolved Hide resolved
end


properties (Access=public)
MetricReader
end
Expand Down Expand Up @@ -44,27 +45,6 @@
obj.MetricReader = reader;
dnarula-mw marked this conversation as resolved.
Show resolved Hide resolved
end


function meter = getMeter(obj, mtname, mtversion, mtschema)
arguments
obj
mtname
mtversion = ""
mtschema = ""
end
% name, version, schema accepts any types that can convert to a
% string
import opentelemetry.common.mustBeScalarString
mtname = mustBeScalarString(mtname);
mtversion = mustBeScalarString(mtversion);
mtschema = mustBeScalarString(mtschema);
id = obj.Proxy.getMeter(mtname, mtversion, mtschema);
Meterproxy = libmexclass.proxy.Proxy("Name", ...
"libmexclass.opentelemetry.MeterProxy", "ID", id);
meter = opentelemetry.metrics.Meter(Meterproxy, mtname, mtversion, mtschema);
end


function addMetricReader(obj, reader)
arguments
obj
Expand Down
8 changes: 3 additions & 5 deletions sdk/metrics/include/opentelemetry-matlab/sdk/metrics/MeterProviderProxy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "opentelemetry/sdk/metrics/push_metric_exporter.h"

#include "opentelemetry-matlab/metrics/MeterProxy.h"
#include "opentelemetry-matlab/metrics/MeterProviderProxy.h"

namespace metrics_api = opentelemetry::metrics;
namespace nostd = opentelemetry::nostd;
Expand All @@ -32,17 +33,14 @@ namespace otlpexporter = opentelemetry::exporter::otlp;


namespace libmexclass::opentelemetry::sdk {
class MeterProviderProxy : public libmexclass::proxy::Proxy {
class MeterProviderProxy : public libmexclass::opentelemetry::MeterProviderProxy {
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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I will look into this, thanks.

REGISTER_METHOD(MeterProviderProxy, addMetricReader);
}

static libmexclass::proxy::MakeResult make(const libmexclass::proxy::FunctionArguments& constructor_arguments);

void getMeter(libmexclass::proxy::method::Context& context);

void addMetricReader(libmexclass::proxy::method::Context& context);

protected:
Expand Down
28 changes: 0 additions & 28 deletions sdk/metrics/src/MeterProviderProxy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,32 +27,6 @@ libmexclass::proxy::MakeResult MeterProviderProxy::make(const libmexclass::proxy
return out;
}

void MeterProviderProxy::getMeter(libmexclass::proxy::method::Context& context) {
// Always assumes 3 inputs
matlab::data::StringArray name_mda = context.inputs[0];
std::string name = static_cast<std::string>(name_mda[0]);
matlab::data::StringArray version_mda = context.inputs[1];
std::string version = static_cast<std::string>(version_mda[0]);
matlab::data::StringArray schema_mda = context.inputs[2];
std::string schema = static_cast<std::string>(schema_mda[0]);

auto mt = CppMeterProvider->GetMeter(name, version, schema);

// instantiate a MeterProxy instance
MeterProxy* newproxy = new MeterProxy(mt);
auto mtproxy = std::shared_ptr<libmexclass::proxy::Proxy>(newproxy);

// obtain a proxy ID
libmexclass::proxy::ID proxyid = libmexclass::proxy::ProxyManager::manageProxy(mtproxy);

// return the ID
matlab::data::ArrayFactory factory;
auto proxyid_mda = factory.createScalar<libmexclass::proxy::ID>(proxyid);
context.outputs[0] = proxyid_mda;
}



void MeterProviderProxy::addMetricReader(libmexclass::proxy::method::Context& context) {
matlab::data::TypedArray<uint64_t> readerid_mda = context.inputs[0];
libmexclass::proxy::ID readerid = readerid_mda[0];
Expand All @@ -64,6 +38,4 @@ void MeterProviderProxy::addMetricReader(libmexclass::proxy::method::Context& co
}




} // namespace libmexclass::opentelemetry
34 changes: 34 additions & 0 deletions test/tmetrics.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,6 +537,40 @@ function testHistogramDelta(testCase)
verifyEqual(testCase, str2double(counts{len}), sum(currentvals>bounds(len-1)));
end
end

function testGetSetMeterProvider(testCase)
% testGetSetMeterProvider: setting and getting global instance of MeterProvider
exporter = opentelemetry.exporters.otlp.OtlpHttpMetricExporter(...
"PreferredAggregationTemporality", "Delta");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need "Delta" temporality here. I would just call the exporter constructor with no input and use the default temporality.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did the tests fail? What are the error messages?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

reader = opentelemetry.sdk.metrics.PeriodicExportingMetricReader(exporter, ...
"Interval", seconds(2), "Timeout", seconds(1));
mp = opentelemetry.sdk.metrics.MeterProvider(reader);
setMeterProvider(mp);

metername = "foo";
countername = "bar";
m = getMeter(mp, metername);
dnarula-mw marked this conversation as resolved.
Show resolved Hide resolved
c = createCounter(m, countername);

% create testing value
val = 10;

% add value and attributes
c.add(val);

pause(2.5);

% perform test comparisons
clear mp;
results = readJsonResults(testCase);
results = results{1};
% check a counter has been created, and check its resource to identify the
% correct MeterProvider has been used
verifyNotEmpty(testCase, results);

verifyEqual(testCase, string(results.resourceMetrics.scopeMetrics.metrics.name), countername);
verifyEqual(testCase, string(results.resourceMetrics.scopeMetrics.scope.name), metername);
end

end

Expand Down