Initial folder layout #8

reyang · 2019-10-17T19:06:32Z

This is the proposed folder layout according to the spec.

I've always been bad at (or at least struggling with) naming things. For example:

Should I use opentelemetry_api/opentelemetry/context or just api/context? I imagine the C++ namespace would be opentelemetry.context rather than opentelemetry.api.context.
Should I put examples folder in both API and SDK, or put examples at top level? I think tests should split into API/SDK folder, but not sure about examples.
Do we want separate documents for API and SDK?

Would be happy to get more perspectives.

g-easy · 2019-10-18T06:05:25Z

How about opentelemetry_api/opentelemetry/context -> opentelemetry/api/context

What is this going to look like from the consumer side? e.g.:

// my_lib.cc
#include "opentelemetry/api/context/context.h"

void Run() {
  opentelemetry::context::Context ctx = opentelemetry::context::CurrentContext();
  ctx.span().add_annotation("I'm running!");
}

And the build?

# bazel BUILD file
cc_library(
  name = "my_lib",
  srcs = ["my_lib.cc"],
  deps = ["@io_opentelemetry_cpp//opentelemetry/api/context:context"]
)

reyang · 2019-10-18T17:01:32Z

How about opentelemetry_api/opentelemetry/context -> opentelemetry/api/context

This is what's in my mind (assuming opentelemetry API package is installed to the default include folder):

#include <opentelemetry/context/context.h>

void Run() {
  opentelemetry::context::Context ctx = opentelemetry::context::CurrentContext();
  ctx.span().add_annotation("I'm running!");
}

I wish to avoid api from the consumption side as the consumer is always using the API.
It might be okay to have opentelemetry::sdk, similar like what we've done in Python.

jmacd · 2019-10-18T18:54:55Z

This latest code snippet looks good to me.

g-easy · 2019-10-21T02:32:44Z

I like the snippet in #8 (comment)

Is there an advantage to calling the parent dir opentelemetry_api instead of just api?

In a bazel-style build, the target would look like one of:

"@io_opentelemetry_cpp//opentelemetry_api/opentelemetry/context:context"
"@io_opentelemetry_cpp//api/opentelemetry/context:context"

In a make install situation I'm guessing there would be no difference since you would copy the {opentelemetry_api,api}/opentelemetry subdir to /usr/local/include.

g-easy · 2019-10-21T02:35:11Z

Should we standardize on underscores instead of hyphens in filenames and directories?

In particular, hyphens make interop with python impossible because it's a syntax error to import otel-api.context

rnburn · 2019-10-21T05:36:16Z

I'd recommend choosing a directory structure that clearly separates the public header files from the source code and internal headers.

One popular way of organizing would be to do something more like this

opentelemetry-api/include/opentelmetry/api/... <- public installed headers go here
opentelemetry-api/src  <- source or internal headers go here (or empty if the api is header-only)
opentelemetry-sdk/include/opentelemetry/sdk/...
opentelemetry-sdk/src/...

All the boost projects are organized in this fashion (see, for example, the layout of boost-filesystem).

Even Envoy (which doesn't install header files) has this same sort separation for header files that act more like a public interface.

Or, if the opentelemetry-api and opentelemetry-sdk don't function like isolated subprojects, maybe do something like this

include/opentelemetry/api/...
include/opentelemetry/sdk/...
src/api/...
src/sdk/...
CMakeLists.txt <- defaults to install both an sdk and api, but has options to install just the api
BUILD

rnburn · 2019-10-21T05:49:07Z

Where would the top-level CMakeLists.txt file go? Does it go in the root directory?

If I were to do something like this

git clone https://github.com/open-telemetry/opentelemetry-cpp.git
cd opentelemetry-cpp
cmake .
make && make install

Should that install both the api and an sdk with a default tracer? Or would I need to use separate cmake commands?

g-easy · 2019-10-21T06:44:45Z

Re #8 (comment): I agree about keeping private headers separate. In OpenCensus we use an internal/ subdirectory.

How about:

include/opentelemetry/ # for api
include/opentelemetry/sdk/ # for sdk

Re #8 (comment): I would prefer to keep CMakeLists.txt and WORKSPACE in the root of the repo, yes. If someone has a compelling reason not to do this, please speak up.

As for selecting which components to build, we could make those CMake option()s.

reyang · 2019-10-21T15:58:44Z

How about:

include/opentelemetry/ # for api
include/opentelemetry/sdk/ # for sdk

We might want to separate the sdk to a totally different folder, and have a separate build. In this way we allow people to follow the standard sdk and make their own SDK implementation.

reyang · 2019-10-21T16:04:17Z

Re #8 (comment): I would prefer to keep CMakeLists.txt and WORKSPACE in the root of the repo, yes. If someone has a compelling reason not to do this, please speak up.

As for selecting which components to build, we could make those CMake option()s.

Being able to build everything from a single command is great! I think most of the actual build logic would be specific to API and SDK, the top level CMakeList.txt and WORKSPACE are just a wrapper to include API and SDK (so one should be able to easily build the API without SDK).

reyang · 2019-10-21T16:07:26Z

Should that install both the api and an sdk with a default tracer? Or would I need to use separate cmake commands?

One command would be great and we should try to achieve it. Note that installing the default SDK implementation doesn't mean that the SDK default tracer will be used. User has to explicitly specify the SDK tracer, otherwise it will be using the no-op implementation provided by the API package.

maxgolov · 2019-10-21T22:10:12Z

I would propose something like this for a layout:

.github						- github internal settings
bazel						- bazel build configuration files/scripts
cmake						- cmake build configuration files/scripts
docs						- documentation (markdown)
include (?)					- public headers
examples					- location for examples
third_party (?)					- third party libraries the SDK depends on
platforms					- platform-specific layer implementation
ext? | modules? | lib/modules?			- extensions or modules
testing						- test infra
+ gtest
tools
+ vcpkg
.clang-format

g-easy · 2019-10-22T01:08:01Z

Summarizing what we discussed in today's meeting:

We are sort of shipping two separate modules (api, sdk) in one repo (opentelemetry-cpp).
For convenience we are somewhat conflating their build systems so that people don't have to build/install them separately. (we can change this in the future)
We need to be careful that e.g. api doesn't accidentally grow dependencies on sdk.

Proposed layout:

README.md
CMakeLists.txt <- has options for what to build
WORKSPACE
BUILD
bazel/
cmake/ <- common cmake helpers
docs/
examples/
.clang-format
opentelemetry-api/
  include/
    opentelemetry/ <- copied to /usr/local/include
      context/
        context.h <-- public header of API
      trace/
        span_context.h
  src/
    opentelemetry/
      context/ <- don't need this level of granularity in src
        context.cc
        context_impl.h <- private header, not part of user API
  test/ <- doesn't end up in libopentelemetry.so
    opentelemetry/
      context/ <- don't need this level of granularity in src
        context_test.cc
        context_test_utils.cc
opentelemetry-sdk/
  include/
    opentelemetry/ <- also copied to /usr/local/include
      sdk/
        resource/
          resource.h <- public header of sdk
  src/ <- same as above
    opentelemetry/
      resource/
        resource.cc
  test/ <- same as above

Some rationale:

Keep include files in a separate tree. This makes them easier to install and makes it super clear which headers are public, and therefore part of an API that might have stability guarantees.
Keep tests in a separate tree from sources. This makes it clear that this code is not part of the public API (e.g. test helper classes) and also that none of this code should end up in a binary artifact like a libopentelemetry-api.so
How to find things? If your code is in src/opentelemetry/foo/bar.cc then your tests should be in test/opentelemetry/foo/bar_test.cc

jmacd

Looks good. I only wonder if we could shorten opentelemetry-api and opentelemetry-sdk to api and sdk.

g-easy · 2019-10-25T02:18:52Z

I agree with short api/ and sdk/ dir names.

reyang · 2019-10-25T02:22:03Z

Looks good. I only wonder if we could shorten opentelemetry-api and opentelemetry-sdk to api and sdk.

I agree with short api/ and sdk/ dir names.

Do we plan to have exporters and other plugins/extensions in this repo? If not, having names like api and sdk would be great. If we do plan to have other packages in this repo, we need to consider the folder structure right now.

g-easy · 2019-10-25T02:24:56Z

IMO start with exporters in the same repo and split them out later if it makes sense to do so.

I see https://github.com/open-telemetry/opentelemetry-java has exporters/ and contrib/ directories.

reyang · 2019-10-25T02:26:51Z

IMO start with exporters in the same repo and split them out later if it makes sense to do so.

I see https://github.com/open-telemetry/opentelemetry-java has exporters/ and contrib/ directories.

That works for me.

Origin/propagators

Fix observable attributes drop (open-telemetry#2557)

initial folder layout

4b1772a

reyang requested review from g-easy, jmacd, maxgolov, rnburn and tigrannajaryan as code owners October 17, 2019 19:06

better align with the specification

c6013a4

reyang closed this Oct 21, 2019

reyang reopened this Oct 21, 2019

jmacd approved these changes Oct 24, 2019

View reviewed changes

reyang added 2 commits October 24, 2019 19:12

adjust folder layout based on SIG meeting result

89e24e3

remove API/SDK specific README for now

8c1f241

simplify folder names

18b552d

g-easy approved these changes Oct 25, 2019

View reviewed changes

reyang merged commit 976bc27 into master Oct 25, 2019

reyang deleted the layout branch November 4, 2019 21:02

0x4b pushed a commit to 0x4b/opentelemetry-cpp that referenced this pull request Jul 31, 2020

Merge pull request open-telemetry#8 from Tianlin-Zhao/origin/propagators

60d95a7

Origin/propagators

ZDzhao mentioned this pull request Feb 18, 2022

crash in ext::http::client::curl::HttpOperation::Send #1220

Closed

mcarajatchawla mentioned this pull request Oct 13, 2022

Jaeger Exporter Compilation error #1680

Closed

GerHobbelt pushed a commit to GerHobbelt/opentelemetry-cpp that referenced this pull request Jun 21, 2024

Merge pull request open-telemetry#8 from open-telemetry/main

0ca7490

Fix observable attributes drop (open-telemetry#2557)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial folder layout #8

Initial folder layout #8

reyang commented Oct 17, 2019

g-easy commented Oct 18, 2019

reyang commented Oct 18, 2019

jmacd commented Oct 18, 2019

g-easy commented Oct 21, 2019

g-easy commented Oct 21, 2019

rnburn commented Oct 21, 2019 •

edited

Loading

rnburn commented Oct 21, 2019 •

edited

Loading

g-easy commented Oct 21, 2019

reyang commented Oct 21, 2019

reyang commented Oct 21, 2019

reyang commented Oct 21, 2019

maxgolov commented Oct 21, 2019 •

edited

Loading

g-easy commented Oct 22, 2019

jmacd left a comment

g-easy commented Oct 25, 2019

reyang commented Oct 25, 2019

g-easy commented Oct 25, 2019

reyang commented Oct 25, 2019

Initial folder layout #8

Initial folder layout #8

Conversation

reyang commented Oct 17, 2019

g-easy commented Oct 18, 2019

reyang commented Oct 18, 2019

jmacd commented Oct 18, 2019

g-easy commented Oct 21, 2019

g-easy commented Oct 21, 2019

rnburn commented Oct 21, 2019 • edited Loading

rnburn commented Oct 21, 2019 • edited Loading

g-easy commented Oct 21, 2019

reyang commented Oct 21, 2019

reyang commented Oct 21, 2019

reyang commented Oct 21, 2019

maxgolov commented Oct 21, 2019 • edited Loading

g-easy commented Oct 22, 2019

jmacd left a comment

Choose a reason for hiding this comment

g-easy commented Oct 25, 2019

reyang commented Oct 25, 2019

g-easy commented Oct 25, 2019

reyang commented Oct 25, 2019

rnburn commented Oct 21, 2019 •

edited

Loading

rnburn commented Oct 21, 2019 •

edited

Loading

maxgolov commented Oct 21, 2019 •

edited

Loading