Metrics SDK: View API #1110

lalitb · 2021-12-03T08:37:30Z

Fixes #1088

Changes

Implements View API: View, View Registry, InstrumentSelector, MeterSelector
Placeholder for Aggregator and Aggregation.

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

…-cpp into metrics-sdk-views-2

codecov · 2021-12-06T21:23:39Z

Codecov Report

Merging #1110 (5e14816) into main (de276f5) will decrease coverage by 0.11%.
The diff coverage is 88.33%.

@@            Coverage Diff             @@
##             main    #1110      +/-   ##
==========================================
- Coverage   93.36%   93.26%   -0.10%     
==========================================
  Files         165      173       +8     
  Lines        6233     6362     +129     
==========================================
+ Hits         5819     5933     +114     
- Misses        414      429      +15

Impacted Files	Coverage Δ
...ntelemetry/sdk/metrics/view/attributes_processor.h	`0.00% <0.00%> (ø)`
...clude/opentelemetry/sdk/metrics/view/aggregation.h	`33.34% <33.34%> (ø)`
sdk/include/opentelemetry/sdk/metrics/view/view.h	`63.64% <63.64%> (ø)`
...include/opentelemetry/sdk/metrics/view/predicate.h	`72.73% <72.73%> (ø)`
...opentelemetry/sdk/metrics/view/predicate_factory.h	`77.78% <77.78%> (ø)`
...ude/opentelemetry/sdk/metrics/view/view_registry.h	`93.55% <93.55%> (ø)`
...entelemetry/sdk/metrics/view/instrument_selector.h	`100.00% <100.00%> (ø)`
...de/opentelemetry/sdk/metrics/view/meter_selector.h	`100.00% <100.00%> (ø)`
sdk/src/metrics/meter_context.cc	`40.00% <100.00%> (ø)`
sdk/src/metrics/meter_provider.cc	`78.79% <100.00%> (+0.67%)`	⬆️
... and 4 more

…-cpp into metrics-sdk-views-2

lalitb · 2021-12-13T08:32:28Z

@jsuereth - As @reyang is on vacation this month, would need your help on reviewing this. This is still not the final code and will change as we progress further on Aggregation and Aggregator APIs.

jsuereth · 2021-12-14T22:29:42Z

sdk/include/opentelemetry/sdk/metrics/view/predicate.h

+#  if HAVE_WORKING_REGEX
+      return std::regex_match(str.data(), reg_key_);
+#  else
+      return false;  // not supported


Seems like this should fail in some fashion.

Yes, this will be supported eventually. For now, we can add the LOG_ERROR to log the error while returning false.

Added LOG_ERROR. Let me know if you have a better way to fail. This will eventually return a default View. We don't throw exceptions in sdk. Let me know if you have a better way to fail.

jsuereth

Overall looks great!

One major comment on default view name/description (the default metric from an instrument with new view is its "natural" aggregation (marked TODO) and the instrument name/description)
A couple questions/nits that can get resolved as implementation/benchmarking land to think about.

jsuereth · 2021-12-15T15:25:26Z

sdk/include/opentelemetry/sdk/metrics/view/attributes_processor.h

+
+class DefaultAttributesProcessor : public AttributesProcessor
+{
+  MetricAttributes process(


IIUC - by necessity, this API will need to take in an Iterator and return a concrete object. It's likely this would get used on the "hot path" of metrics. Does it make sense to have this API work only against AttributeMap, that way in the event you're just passing it through, there isn't a sort + unique step (I forget the details of attribute map).

Yes, your understanding is correct here. The metrics API takes non-owning KeyValueIterable object as here, and the plan is to make a copy of this only when required in hot-path. Attribute-processor could be one of the places to create a copy in case of passthrough.

I expect this to (commonly) be a passthrough case. Can deal with it after initial implementation :)

jsuereth · 2021-12-15T15:27:23Z

sdk/include/opentelemetry/sdk/metrics/view/predicate.h

+public:
+  PatternPredicate(opentelemetry::nostd::string_view pattern) : reg_key_{pattern.data()}
+  {
+    if (pattern == "*")


I don't think you need to support * in all your predicates. You could have ONE predicate which is the "star" predicate and then a "predicate factory" that returns either a new PatternPredicate or MatchEverything predicate, if you wanted.

Only commenting because I noticed this logic duplicate in the "ExactPredicate" code as well.

Good point. Will change accordingly.

This is changed now.

jsuereth · 2021-12-15T15:29:02Z

sdk/include/opentelemetry/sdk/metrics/view/view_registry.h

+    // return default view if none found;
+    if (!found)
+    {
+      static View view(kDefaultViewName, kDefaultViewDescription);


[blocking] The default name and description should come from the instrument descriptor's name/description.

Should we instead return default view ( with empty name and description ) as done in Java here ? This will allow us to return a default static immutable object always when a view is not found.

Have made changes as per my suggestion above. Let me know if that works.

Ah, I see, you're planning to then select the name of the metric by checking for empty description or empty name.

I understand wanting to keep this static. You'll need to either use Java's "MetricDescriptor" trick to figure out what to name the underlying metric, or keep this logic elsewhere.

That's entirely reasonable to do, but remember you'll still be instantiating something to remember the name/description of the resulting metric. For C++ it may be easier to do so view the View object, but you can decide once you work on the "storage" aspect of views :)

jsuereth · 2021-12-16T21:12:22Z

sdk/include/opentelemetry/sdk/metrics/view/view_registry.h

+    // return default view if none found;
+    if (!found)
+    {
+      static View view("");


Shouldn't this still pull name/description from the instrument?

initial view commit

c7b92a8

lalitb requested a review from a team December 3, 2021 08:37

lalitb marked this pull request as draft December 3, 2021 08:37

lalitb and others added 6 commits December 3, 2021 00:37

Merge branch 'main' into metrics-sdk-views-2

98f94e0

build fix

a6866c6

Merge branch 'metrics-sdk-views-2' of github.com:lalitb/opentelemetry…

0d49214

…-cpp into metrics-sdk-views-2

build issue

c41471a

regex for gcc4.8

7a7a57a

Merge branch 'main' into metrics-sdk-views-2

34ef0c5

lalitb and others added 10 commits December 6, 2021 13:52

fix 4.8 build

514bd85

Merge branch 'metrics-sdk-views-2' of github.com:lalitb/opentelemetry…

8af6f2f

…-cpp into metrics-sdk-views-2

fix test

4f84e35

more changes

9111528

Merge branch 'main' into metrics-sdk-views-2

189f293

add tests for view_registry

f3e9172

Merge branch 'metrics-sdk-views-2' of github.com:lalitb/opentelemetry…

c013f68

…-cpp into metrics-sdk-views-2

more tests for view_registry

c537f8b

add const variable:

f2f412b

disable predicate test for gcc4.8

918e7f2

lalitb marked this pull request as ready for review December 8, 2021 21:07

lalitb changed the title ~~[WIP] Metrics SDK: View API~~ Metrics SDK: View API Dec 8, 2021

lalitb mentioned this pull request Dec 10, 2021

[WIP] Aggregation API #1129

Closed

3 tasks

lalitb and others added 2 commits December 13, 2021 00:05

Aggregation API should return reference to Aggregator

0bd982d

Merge branch 'main' into metrics-sdk-views-2

0ac15a2

Merge branch 'main' into metrics-sdk-views-2

0a5863e

jsuereth reviewed Dec 14, 2021

View reviewed changes

jsuereth reviewed Dec 15, 2021

View reviewed changes

Merge branch 'main' into metrics-sdk-views-2

7f91b4e

lalitb and others added 5 commits December 15, 2021 17:59

review comments

bcb43c6

log error for regex for gcc4.8

2bfb652

compile error

cb4d314

more compiler error

447cece

Merge branch 'main' into metrics-sdk-views-2

c28426b

jsuereth reviewed Dec 16, 2021

View reviewed changes

jsuereth approved these changes Dec 16, 2021

View reviewed changes

ThomsonTan and others added 2 commits December 17, 2021 09:46

Merge branch 'main' into metrics-sdk-views-2

31f681d

Merge branch 'main' into metrics-sdk-views-2

5e14816

lalitb merged commit 09fb4e0 into open-telemetry:main Dec 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metrics SDK: View API #1110

Metrics SDK: View API #1110

lalitb commented Dec 3, 2021 •

edited

Loading

codecov bot commented Dec 6, 2021 •

edited

Loading

lalitb commented Dec 13, 2021 •

edited

Loading

jsuereth Dec 14, 2021

lalitb Dec 15, 2021

lalitb Dec 16, 2021

jsuereth left a comment

jsuereth Dec 15, 2021

lalitb Dec 15, 2021

jsuereth Dec 16, 2021

jsuereth Dec 15, 2021

lalitb Dec 15, 2021

lalitb Dec 16, 2021

jsuereth Dec 15, 2021

lalitb Dec 15, 2021 •

edited

Loading

lalitb Dec 16, 2021

jsuereth Dec 16, 2021

jsuereth Dec 16, 2021

Metrics SDK: View API #1110

Metrics SDK: View API #1110

Conversation

lalitb commented Dec 3, 2021 • edited Loading

Changes

codecov bot commented Dec 6, 2021 • edited Loading

Codecov Report

lalitb commented Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsuereth 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

lalitb Dec 15, 2021 • 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

lalitb commented Dec 3, 2021 •

edited

Loading

codecov bot commented Dec 6, 2021 •

edited

Loading

lalitb commented Dec 13, 2021 •

edited

Loading

lalitb Dec 15, 2021 •

edited

Loading