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

fix: issue 368- consistent namespace scope resolution #1008

Conversation

PaurushGarg
Copy link
Member

@PaurushGarg PaurushGarg commented Oct 4, 2021

Fixes Issue (368)
In .cc files across whole project, opentelemetry::* is simplified to * , to follow the namespace guidelines. To do this, following
namespace aliases were used, to establish convention:

  • namespace common = opentelemetry::common;
  • namespace nostd = opentelemetry::nostd;
  • namespace trace = opentelemetry::trace;
  • namespace trace_api = opentelemetry::trace;
  • namespace resource = opentelemetry::sdk::resource;
  • namespace exporter_trace = opentelemetry::exporter::trace;
  • namespace context = opentelemetry::context;
  • namespace metrics_api = opentelemetry::metrics;
  • namespace sdkmetrics = opentelemetry::sdk::metrics;
  • namespace exportermetrics = opentelemetry::exporter::metrics;
  • namespace trace_sdk = opentelemetry::sdk::trace;
  • namespace sdktrace = opentelemetry::sdk::trace;
  • namespace exporterlogs = opentelemetry::exporter::logs;
  • namespace exportertrace = opentelemetry::exporter::trace;
  • namespace proto = opentelemetry::proto;
  • namespace context = opentelemetry::context;
  • namespace otlp = opentelemetry::exporter::otlp;
  • namespace plugin = opentelemetry::plugin;
  • namespace curl = opentelemetry::ext::http::client::curl;
  • namespace http_client = opentelemetry::ext::http::client;
  • namespace logs_api = opentelemetry::logs;
  • namespace sdklogs = opentelemetry::sdk::logs;

Note: trace and trace_api both are used for opentelemetry::trace;
trace_sdk and sdktrace both are used for opentelemetry::sdk::trace; These were already existing in the code.

Tracking Issue #368

Please provide a brief description of the changes here.
Usage of scope resolution was made consistent across all files using namespace convention and guidelines.

@PaurushGarg PaurushGarg requested a review from a team October 4, 2021 16:21
@codecov
Copy link

codecov bot commented Oct 4, 2021

Codecov Report

Merging #1008 (2a1d460) into main (b2a0a56) will not change coverage.
The diff coverage is 90.53%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1008   +/-   ##
=======================================
  Coverage   94.41%   94.41%           
=======================================
  Files         158      158           
  Lines        6077     6077           
=======================================
  Hits         5737     5737           
  Misses        340      340           
Impacted Files Coverage Δ
api/include/opentelemetry/trace/noop.h 86.21% <ø> (ø)
sdk/include/opentelemetry/sdk/trace/sampler.h 100.00% <ø> (ø)
...lude/opentelemetry/sdk/trace/samplers/always_off.h 80.00% <ø> (ø)
...clude/opentelemetry/sdk/trace/samplers/always_on.h 100.00% <ø> (ø)
sdk/src/trace/samplers/parent.cc 100.00% <ø> (ø)
sdk/test/trace/always_on_sampler_test.cc 91.67% <ø> (ø)
api/test/trace/provider_test.cc 80.00% <33.34%> (ø)
...t/src/http/client/curl/http_client_factory_curl.cc 50.00% <50.00%> (ø)
exporters/ostream/src/span_exporter.cc 90.33% <72.73%> (ø)
...lemetry/ext/http/client/curl/http_operation_curl.h 78.09% <73.34%> (ø)
... and 20 more

@lalitb
Copy link
Member

lalitb commented Oct 14, 2021

Thank you for the PR. I can see it had to go through lots of iteration to make the build work. Changes look good in general, couple of minor comments:

  • We can be consistent in namespace alias names:
namespace trace_sdk = opentelemetry::sdk::trace;
namespace sdktrace = opentelemetry::sdk::trace;

If we are using trace_api or trace for api, let's use trace_sdk, metrics_sdk for sdk.

  • In header files, there are few namespace aliases that are unknowingly becoming part of public API (namespace pollution). few are here. Do you think it would be easy to remove them ?
labhas@DESKTOP-D0BLHPQ:~/obs/ot/opentelemetry-cpp/api$ find . -name "*.h" | xargs grep 'namespace ' | grep '='
./include/opentelemetry/trace/span_context.h:namespace trace_api = opentelemetry::trace;
labhas@DESKTOP-D0BLHPQ:~/obs/ot/opentelemetry-cpp/api$ cd ../sdk
labhas@DESKTOP-D0BLHPQ:~/obs/ot/opentelemetry-cpp/sdk$ find . -name "*.h" | xargs grep 'namespace ' | grep '='
./src/trace/span.h:namespace trace_api = opentelemetry::trace;
./include/opentelemetry/sdk/metrics/processor.h:namespace sdkmetrics = opentelemetry::sdk::metrics;
./include/opentelemetry/sdk/metrics/controller.h:namespace metrics_api = opentelemetry::metrics;
./include/opentelemetry/sdk/metrics/record.h:namespace metrics_api = opentelemetry::metrics;
./include/opentelemetry/sdk/metrics/aggregator/aggregator.h:namespace metrics_api = opentelemetry::metrics;
./include/opentelemetry/sdk/metrics/aggregator/min_max_sum_count_aggregator.h:namespace metrics_api = opentelemetry::metrics;
./include/opentelemetry/sdk/metrics/aggregator/sketch_aggregator.h:namespace metrics_api = opentelemetry::metrics;
./include/opentelemetry/sdk/metrics/aggregator/gauge_aggregator.h:namespace metrics_api = opentelemetry::metrics;
./include/opentelemetry/sdk/metrics/aggregator/exact_aggregator.h:namespace metrics_api = opentelemetry::metrics;
./include/opentelemetry/sdk/metrics/aggregator/histogram_aggregator.h:namespace metrics_api = opentelemetry::metrics;
./include/opentelemetry/sdk/metrics/aggregator/counter_aggregator.h:namespace metrics_api = opentelemetry::metrics;
./include/opentelemetry/sdk/metrics/sync_instruments.h:namespace metrics_api = opentelemetry::metrics;
./include/opentelemetry/sdk/metrics/meter.h:namespace metrics_api = opentelemetry::metrics;
./include/opentelemetry/sdk/metrics/instrument.h:namespace metrics_api = opentelemetry::metrics;
./include/opentelemetry/sdk/metrics/async_instruments.h:namespace metrics_api = opentelemetry::metrics;
./include/opentelemetry/sdk/trace/samplers/always_off.h:namespace trace_api = opentelemetry::trace;
./include/opentelemetry/sdk/trace/samplers/always_on.h:namespace trace_api = opentelemetry::trace;
./include/opentelemetry/sdk/trace/samplers/trace_id_ratio.h:namespace trace_api = opentelemetry::trace;
./include/opentelemetry/sdk/trace/samplers/parent.h:namespace trace_api = opentelemetry::trace;
./include/opentelemetry/sdk/trace/sampler.h:namespace trace_api = opentelemetry::trace;
labhas@DESKTOP-D0BLHPQ:~/obs/ot/opentelemetry-cpp/sdk$

@lalitb
Copy link
Member

lalitb commented Oct 25, 2021

@PaurushGarg - Just wondering if you have gone through the earlier comments and any suggestions on it.

@ThomsonTan
Copy link
Contributor

@PaurushGarg let us know if you will get a chance to review all the comments and resolve the conflict, or need our help on it.

@lalitb lalitb mentioned this pull request Nov 4, 2021
3 tasks
@PaurushGarg
Copy link
Member Author

@PaurushGarg - Just wondering if you have gone through the earlier comments and any suggestions on it.

@lalitb Sorry it took some time. I have updated both the comments regarding trace_sdk and metric_sdk, and also updated the header files to remove the namespace aliases.

@PaurushGarg
Copy link
Member Author

@PaurushGarg let us know if you will get a chance to review all the comments and resolve the conflict, or need our help on it.

@ThomsonTan Sorry it took some time. I have updated both the comments regarding trace_sdk and metric_sdk, and also updated the header files to removes the namespace aliases.

@ThomsonTan
Copy link
Contributor

@lalitb do you have any other comments?

@lalitb
Copy link
Member

lalitb commented Nov 16, 2021

@lalitb do you have any other comments?

No, it looks good now. Thanks, @PaurushGarg for your efforts.

@lalitb lalitb merged commit 9772156 into open-telemetry:main Nov 16, 2021
@PaurushGarg PaurushGarg deleted the fix_368_consistent_namespace_scope_resolution branch December 18, 2021 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants