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.

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

Added docs/library-distribution.md #38

Merged
merged 9 commits into from
Mar 16, 2020

Conversation

eyakimov-bbg
Copy link
Contributor

As per the discussion on the SIG meeting on Feb 4th 2020, I've started to write up
the analysis and recommendations of the library distribution strategy.

This aims to address some of the questions raised in #4 and most recently in #37.

The idea is to summarise the current understanding and implications in this document.

I've left a placeholder to summarise the constraints for guaranteeing ABI, perhaps @rnburn can populate this as he's been move knowledgeable in this area so far.

I wanted to share this while its still in WIP to get further input.

Started to clarify how the library can be distributed
@eyakimov-bbg eyakimov-bbg force-pushed the docs-library-distribution branch from a86b79a to 7735f3e Compare February 21, 2020 22:22
### Binary vs Source Distribution

The OpenTelemetry-cpp libraries (API and SDK) and any library that depends on
them are expected to be distributed as either "source" or "binary" artefacts.
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
them are expected to be distributed as either "source" or "binary" artefacts.
them are expected to be distributed as either "source" or "binary" artifacts.

different binding models and the recommendations.

- **OpenTelemetry Libraries** - The libraries produced from this project,
the API library will contain some globals/singletons such as `TracerProvider`.
Copy link
Member

Choose a reason for hiding this comment

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

The API library will also contain implementations of distributed tracing and basic in-proc context propagation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reworded this to include "minimal implementation" as is the terminology used in the reference documentation in the specification. I wanted to focus on the characteristics relevant to the context (i.e. compiled binary artifacts) rather than thoroughly describe the content, but happy to change

@reyang reyang added the documentation Improvements or additions to documentation label Feb 21, 2020
@eyakimov-bbg eyakimov-bbg force-pushed the docs-library-distribution branch from a23fe32 to 5299abd Compare February 22, 2020 22:14
three models.

- **OpenTelemetry Instrumented Libraries**: Can be distributed for linkage with
any of the three models but SHOULD NOT statically link the OpenTelemetry API.
Copy link
Contributor

@rnburn rnburn Feb 24, 2020

Choose a reason for hiding this comment

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

The OpenTelemetry API is header-only so there's no linking.

But to clarify, this is only a problem for certain scenarios. Some examples

  1. Your instrumented library hides the singleton objects in its export map. For instance, in this project Get2 won't be a singleton because it's hidden by the export map.
  2. The app doesn't define the global OpenTelemetry symbols and DSOs are dlopen-ed with RTLD_LOCAL so that their global symbols aren't shared.

In general, I don't think this creates any issues if you're binding the instrumented libraries early.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding notes from weekly SIG discussion:
https://codereview.stackexchange.com/questions/147407/header-only-c-singleton-pattern-implementation
Also the concept of relying on a weak vs strong symbol for "thick" static libraries

with any of the three models but SHOULD NOT statically link the OpenTelemetry
API.

## Example Scenarios
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add scenario for: library with binary distribution that uses OpenTelemetry for instrumentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't that be the Non OpenTelemetry aware application with OpenTelemetry capability library scenario?

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

As we've discussed in the community meeting, I suggest that we merge the skeleton doc so folks can add sections in parallel.

@rnburn
Copy link
Contributor

rnburn commented Mar 16, 2020

I filled out the deployment scenarios and abi-policy.

@g-easy @pyohannes - Does address #37 and #43 ?

@rnburn
Copy link
Contributor

rnburn commented Mar 16, 2020

Looks like auto-formatting is failing on the markdown with buildifier.

@pyohannes
Copy link
Contributor

Does address #37 and #43 ?

Not fully, at least for me.

Abstract classes in the OpenTelemetry API must use ABI stable types in their virtual function
signatures.

In the ABI policy only the API is mentioned, whereas I think the exporter interface is not part of the API layer. From that I'd conclude we don't target ABI stability for exporters.

However, in the library distribution doc SDK and exporters are grouped together in the component section:

  • OpenTelemetry Implementations or Exporters

This leads me to conclude that recommendations and example scenarios are targeted for both SDK implementations and exporters. From that I'd conclude we target the same ABI compatibility policy for SDKs and exporters.

@reyang
Copy link
Member

reyang commented Mar 16, 2020

For typo and small fixes, let's put them in a separate PR.

@reyang reyang merged commit f2c1e99 into open-telemetry:master Mar 16, 2020
@eyakimov-bbg eyakimov-bbg changed the title WIP: Added docs/library-distribution.md Added docs/library-distribution.md Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants