[API] Add InstrumentationScope attributes in MeterProvider::GetMeter() #2224

marcalff · 2023-07-07T15:56:07Z

Fixes #2033

Changes

API:

Implemented a new GetMeter() api with 4 parameters, only available in ABI VERSION 2
Renamed parameters to be consistent with the spec.

CI:

Added a maintainer build to test ABI VERSION 2.

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

Fixes open-telemetry#2033

codecov · 2023-07-07T20:28:00Z

Codecov Report

Merging #2224 (9d0a39a) into main (ad626ce) will decrease coverage by 0.04%.
The diff coverage is 53.85%.

@@            Coverage Diff             @@
##             main    #2224      +/-   ##
==========================================
- Coverage   87.43%   87.39%   -0.04%     
==========================================
  Files         199      199              
  Lines        5997     6009      +12     
==========================================
+ Hits         5243     5251       +8     
- Misses        754      758       +4

Files	Coverage Δ
api/include/opentelemetry/metrics/meter_provider.h	`100.00% <ø> (ø)`
sdk/src/metrics/meter_provider.cc	`86.85% <100.00%> (+0.74%)`	⬆️
api/include/opentelemetry/metrics/noop.h	`37.26% <0.00%> (ø)`
...include/opentelemetry/sdk/common/attribute_utils.h	`90.91% <37.50%> (-7.36%)`	⬇️

... and 2 files with indirect coverage changes

marcalff · 2023-07-07T21:35:21Z

Proof of concept, ready for review and discussion.

This code assumes that #2222 is already merged in main, and is an example of how to deliver fixes with breaking changes.

lalitb · 2023-07-20T20:53:30Z

Thank you for the POC for supporting multiple ABI versions. I don't see any issue with this implementation to support the idea. However, I have concerns about the long-term management of these versions. Given that there may be ABI breaking changes that affect various files and components in the repository, we are using ifdef macros throughout these areas. As time goes on, the code may become increasingly challenging to handle.

The question we need to address is whether it is truly necessary to maintain backward compatibility for ABI compliance. At present, when we talk about ABI compliance for the otel-api, it encompasses the following aspects:

using nostd constructs at the API surface for instrumented libraries that are built with the same otel ABI version but potentially with different compilers or versions.
ensure to increment the ABI version whenever there is a breaking change in otel-api. This ensures that any ABI-only, non-API breaking changes will result in a link error if someone attempts to link libraries that are instrumented with different otel ABI versions. And so, this will prevent runtime errors from occurring.

esigo

Thanks Mark for the great work as always. changes in this PR and #2222 look good.
However, I have similar concern as @lalitb. I think it will be very challenging to maintain backward compatibility. Even if we choose to follow this path, we need to decide how far we are going to be backward compatible. Let's discuss in the next SIG meeting :)

marcalff · 2023-07-26T14:01:31Z

Thanks for looking at the proposal.

Some comments below, organized by topics.

Maintenance complexity

Assume we implement 10 breaking changes, for 10 different unrelated reasons.

The proposal is not to introduce a feature flag for each change,
which would lead to power(2, 10) build configurations to test.

Instead, the proposal is to define only one additional configuration,
ABI v2, that contains these 10 changes, on top of the existing ABI v1.

This point alone makes a huge difference (and, keep in mind that if we do not
adopt the proposal, chances are that we will end up with independent feature flags anyway,
which is much worse).

By the time ABI v2 is introduced,
builds with ABI v1 and ABI v2 are necessary in CI.

Now, as long as ABI v2 is still in experimental status,
there is no strong reason to test all the possible feature combinations
in the experimental ABI v2, the following is sufficient:

multiple CI builds to test ABI v1 combinations (as of today)
a single CI build, with all features enabled, to test ABI v2

A single build in CI is not an issue, and should not be a burden.

By the time ABI v2 is changed from experimental to stable,
it may now contain 15 breaking changes.

IF we want to maintain concurrently ABI v1 and ABI v2 in stable status for
some time, the CI build will then need:

multiple CI builds to test ABI v1 combinations (as of today)
multiple CI builds to test ABI v2 combinations (as of today)

in effect increasing the CI load.

The next logical step is to declare ABI v1 deprecated,
and encourage migration from ABI v1 to ABI v2.

In this phase, the CI build can be simplified to:

a single CI build, with all features enabled, to test deprecated ABI v1
multiple CI builds to test ABI v2 combinations (as of today for ABI v1)

In conclusion, and assuming we repeat the pattern, the complexity at any
time is:

light (one build with all options) CI for deprecated ABI N-1
full (testing all feature options) CI for stable ABI N
light (one build with all options) CI for experimental ABI N+1

Today we have:

full (testing all feature options) CI for frozen and immutable ABI v1

In conclusion, yes, this increases complexity by having more combinations
to support, but to a very limited extend that should be manageable.

Maintain several ABI

A valid discussion point is to decide if we do or do not need to maintain
multiple ABI versions at the same time.

To evaluate this, let's first consider a typical usage scenario,
then see how each solution works.

Use case

Assume that a vendor delivers a library, instrumented for opentelemetry-cpp,
and delivers a binary package.

All versions are for illustration.

(1)

Vendor LIB distributes a package compiled using opentelemetry-cpp 1.10.0.

(2)

Then, a different party, vendor APP, uses the package to build an application,
using the opentelemetry-cpp library available at time of build.

Vendor APP:

links with the package produced by LIB in (1)
configures the opentelemetry-cpp SDK
uses opentelemetry-cpp 1.15.0

Changing the ABI

In this scenario:

ABI v1 as shipped in version 1.10.0 has one binary signature
ABI v1 as shipped in version 1.15.0 has a different binary signature

and the APP crashes.

We all know this is to avoid, which is why no breaking changes are introduced
in stable ABI.

This solution is not viable, the point of this section is to repeat why.

Supporting only one ABI at a time

Assume a breaking change is implemented in opentelemetry-cpp 1.12.0,
which changes the ABI from v1 to v2.

In this scenario:

LIB is compiled against opentelemetry-cpp 1.10.0, using ABI v1
APP is compiled against opentelemetry-cpp 1.15.0, using ABI v2

The code links properly, and does not crash.

At runtime however, LIB is not instrumented,
as all instrumentation calls lands in the noop code shipped by default in ABI
v1.

Setting a global provider singleton in the APP, using in ABI v2,
does not allow LIB, using ABI v1, to see the SDK.

My opinion on this is that this is not viable.

We can't ask applications to link with an old library, like APP using
opentelemetry-cpp 1.10.0 or 1.12.0 in this case.

We can't tell people producing a LIB binary package for a library instrumented for
opentelemetry that their instrumentation may just be ignored,
by no fault of their own, at any point in time ... and we can not ask them to
release a different package again just because a newer ABI was defined.

Supporting multiple ABI

In this case:

When compiling with opentelemetry-cpp 1.10.0, only one ABI is available,
so that LIB is using ABI v1.

When compiling with opentelemetry-cpp 1.15.0, two ABI are available, ABI v1
and ABI v2.

The application, APP, decides to compile against opentelemetry-cpp 1.15.0
using ABI v1.

The application links, does not crash, AND provides instrumentation in LIB.

By the time opentelemetry-cpp 1.20.0 is released, maybe ABI v1 will be
deprecated, so that LIB shipping a newer package for a newer library version
will then decide to use ABI v2 instead.

The point here is that deciding to use one ABI versus another is a choice made
deliberately by the package and/or application at build time, not by opentelemetry-cpp.

marcalff · 2023-09-13T07:02:20Z

Fix blocked by:

[API] Deliver ABI breaking changes #2222

Cleanup

api/include/opentelemetry/metrics/meter_provider.h

Added unit tests.

api/include/opentelemetry/metrics/meter_provider.h

Expanded unit tests

marcalff · 2023-09-26T12:30:14Z

/easycla

lalitb

LGTM. nicely done.

api/include/opentelemetry/metrics/meter_provider.h

lalitb · 2023-09-26T18:43:03Z

sdk/include/opentelemetry/sdk/common/attribute_utils.h

+ attributes->ForEachKeyValue(
+ [&](nostd::string_view key, opentelemetry::common::AttributeValue value) noexcept {
+ SetAttribute(key, value);
+ return true;


nit - some code duplication, which can be moved to a separate utils method. But don't see it really required to fix

marcalff · 2023-09-29T10:36:20Z

@owent @esigo @ThomsonTan

Do you want to have a second look, or ok to merge now ?

owent

LGTM and thanks.

## Changes Update spec-compliance-matrix.md for C++: * `Get a Tracer with scope attributes` implemented by: * open-telemetry/opentelemetry-cpp#2371 * `Links can be recorded after span creation` implemented by: * open-telemetry/opentelemetry-cpp#2380 * `get_meter accepts attributes` implemented by: * open-telemetry/opentelemetry-cpp#2224

marcalff added 2 commits July 7, 2023 17:54

Proof of concept.

fdbef73

Fixes open-telemetry#2033

Fixed instrumentation scope

63fde43

marcalff added 2 commits July 7, 2023 22:51

Cleanup

b1b735f

Code simplification.

d2a107a

marcalff marked this pull request as ready for review July 7, 2023 21:28

marcalff requested a review from a team July 7, 2023 21:28

Merge branch 'main' into fix_breaking_getmeter_2033

e287db5

esigo assigned esigo and lalitb Jul 17, 2023

Merge branch 'main' into fix_breaking_getmeter_2033

a754d54

esigo reviewed Jul 22, 2023

View reviewed changes

Merge branch 'main' into fix_breaking_getmeter_2033

51852cc

marcalff added the issue:blocked Fix blocked, waiting for other fixes as prerequisites label Sep 13, 2023

marcalff mentioned this pull request Sep 13, 2023

[API] Deliver ABI breaking changes #2222

Merged

3 tasks

marcalff removed the issue:blocked Fix blocked, waiting for other fixes as prerequisites label Sep 14, 2023

Merge branch 'main' into fix_breaking_getmeter_2033

88d31da

marcalff changed the title ~~[POC] [Metrics API/SDK] Add InstrumentationScope attributes in MeterProvider::GetMeter()~~ [WIP] [Metrics API/SDK] Add InstrumentationScope attributes in MeterProvider::GetMeter() Sep 14, 2023

marcalff added 3 commits September 20, 2023 20:27

Merge branch 'main' into fix_breaking_getmeter_2033

3853b9d

Added CI to test ABI VERSION 2

b404d40

Cleanup

cleanup

7610dc9

marcalff changed the title ~~[WIP] [Metrics API/SDK] Add InstrumentationScope attributes in MeterProvider::GetMeter()~~ [Metrics API/SDK] Add InstrumentationScope attributes in MeterProvider::GetMeter() Sep 20, 2023

marcalff added pr:please-review This PR is ready for review abi:version_2 Fix is available WITH_ABI_VERSION_2 labels Sep 20, 2023

marcalff mentioned this pull request Sep 20, 2023

Add Synchronous Gauge instrument to the metrics API/SDK #2279

Open

Merge branch 'main' into fix_breaking_getmeter_2033

e3b5f07

marcalff added 2 commits September 22, 2023 08:48

Merge branch 'main' into fix_breaking_getmeter_2033

673af34

Merge branch 'main' into fix_breaking_getmeter_2033

acad9e7

lalitb reviewed Sep 24, 2023

View reviewed changes

api/include/opentelemetry/metrics/meter_provider.h Show resolved Hide resolved

marcalff mentioned this pull request Sep 25, 2023

Prepare release 1.12.0 #2331

Closed

marcalff added 2 commits September 25, 2023 23:24

Implemented support for key value iterable.

d4ec4af

Added unit tests.

Fixed build break.

2125acb

lalitb reviewed Sep 25, 2023

View reviewed changes

api/include/opentelemetry/metrics/meter_provider.h Outdated Show resolved Hide resolved

Resolved overload issue.

8f9f1ec

Expanded unit tests

Cleanup, changelog.

1ac42b6

marcalff changed the title ~~[Metrics API/SDK] Add InstrumentationScope attributes in MeterProvider::GetMeter()~~ [API] Add InstrumentationScope attributes in MeterProvider::GetMeter() Sep 26, 2023

Merge branch 'main' into fix_breaking_getmeter_2033

a53ff4e

lalitb approved these changes Sep 26, 2023

View reviewed changes

marcalff added 2 commits September 27, 2023 09:34

Merge branch 'main' into fix_breaking_getmeter_2033

f812c9c

Merge branch 'main' into fix_breaking_getmeter_2033

9d0a39a

owent approved these changes Sep 30, 2023

View reviewed changes

marcalff merged commit c3c643a into open-telemetry:main Sep 30, 2023
48 checks passed

chenrui333 mentioned this pull request Oct 16, 2023

opentelemetry-cpp 1.12.0 Homebrew/homebrew-core#151221

Merged

marcalff deleted the fix_breaking_getmeter_2033 branch October 27, 2023 10:11

marcalff mentioned this pull request Oct 29, 2023

Update spec-compliance-matrix.md for C++ open-telemetry/opentelemetry-specification#3747

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[API] Add InstrumentationScope attributes in MeterProvider::GetMeter() #2224

[API] Add InstrumentationScope attributes in MeterProvider::GetMeter() #2224

marcalff commented Jul 7, 2023 •

edited

Loading

codecov bot commented Jul 7, 2023 •

edited

Loading

marcalff commented Jul 7, 2023

lalitb commented Jul 20, 2023

esigo left a comment

marcalff commented Jul 26, 2023

marcalff commented Sep 13, 2023

marcalff commented Sep 26, 2023

lalitb left a comment

lalitb Sep 26, 2023 •

edited

Loading

marcalff commented Sep 29, 2023

owent left a comment

[API] Add InstrumentationScope attributes in MeterProvider::GetMeter() #2224

[API] Add InstrumentationScope attributes in MeterProvider::GetMeter() #2224

Conversation

marcalff commented Jul 7, 2023 • edited Loading

Changes

codecov bot commented Jul 7, 2023 • edited Loading

Codecov Report

marcalff commented Jul 7, 2023

lalitb commented Jul 20, 2023

esigo left a comment

Choose a reason for hiding this comment

marcalff commented Jul 26, 2023

Maintenance complexity

Maintain several ABI

Use case

Changing the ABI

Supporting only one ABI at a time

Supporting multiple ABI

marcalff commented Sep 13, 2023

marcalff commented Sep 26, 2023

lalitb left a comment

Choose a reason for hiding this comment

lalitb Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

marcalff commented Sep 29, 2023

owent left a comment

Choose a reason for hiding this comment

marcalff commented Jul 7, 2023 •

edited

Loading

codecov bot commented Jul 7, 2023 •

edited

Loading

lalitb Sep 26, 2023 •

edited

Loading