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.

Sign up for GitHub

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

Jump to bottom

[BUILD] SDK Header files cleanup, use forward declarations #2182

Merged
merged 3 commits into from
Jun 20, 2023
Merged

[BUILD] SDK Header files cleanup, use forward declarations #2182

merged 3 commits into from
Jun 20, 2023

Conversation

cngzhnp
Copy link
Contributor

@cngzhnp cngzhnp commented Jun 6, 2023

Fixes #2043

Changes

Clean up header files in SDK folder to use as much as possible forward declarations.

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

@codecov
Copy link

codecov bot commented Jun 6, 2023 •
edited
Loading

Codecov Report

Merging #2182 (3dbafb8) into main (4cae6aa) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2182      +/-   ##
==========================================
+ Coverage   87.50%   87.53%   +0.03%     
==========================================
  Files         169      169              
  Lines        4887     4888       +1     
==========================================
+ Hits         4276     4278       +2     
+ Misses        611      610       -1
Impacted Files Coverage Δ
...pentelemetry/trace/span_context_kv_iterable_view.h 84.22% <ø> (ø)
exporters/ostream/src/metric_exporter.cc 91.81% <ø> (ø)
exporters/ostream/test/ostream_metric_test.cc 100.00% <ø> (ø)
...include/opentelemetry/sdk/common/attribute_utils.h 98.28% <ø> (ø)
...clude/opentelemetry/sdk/common/attributemap_hash.h 83.88% <ø> (ø)
...include/opentelemetry/sdk/common/circular_buffer.h 100.00% <ø> (ø)
...e/opentelemetry/sdk/common/circular_buffer_range.h 100.00% <ø> (ø)
...nclude/opentelemetry/sdk/common/empty_attributes.h 100.00% <ø> (ø)
...lude/opentelemetry/sdk/common/global_log_handler.h 72.23% <ø> (ø)
...y/sdk/instrumentationscope/instrumentation_scope.h 100.00% <ø> (ø)
... and 65 more

@cngzhnp cngzhnp force-pushed the forward_declaration_for_sdk branch 3 times, most recently from 226ae43 to 2cbbb31 Compare June 6, 2023 22:28
@lalitb
Copy link
Member

lalitb commented Jun 7, 2023 •
edited
Loading

@cngzhnp Just curious, how are you finding out these indirect header includes ?

@cngzhnp cngzhnp marked this pull request as ready for review June 7, 2023 06:32
@cngzhnp cngzhnp requested a review from a team June 7, 2023 06:32
marcalff
marcalff approved these changes Jun 7, 2023
View reviewed changes
Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

Reviewed all 172 files changed.

LGTM, thanks for the cleanup.

@lalitb
Copy link
Member

lalitb commented Jun 9, 2023

@cngzhnp Just curious, how are you finding out these indirect header includes ?

@cngzhnp Could you please reply, this will help review in better way :)

@cngzhnp
Copy link
Contributor Author

cngzhnp commented Jun 9, 2023 •
edited
Loading

@cngzhnp Just curious, how are you finding out these indirect header includes ?

@cngzhnp Could you please reply, this will help review in better way :)

Sorry for the late response @lalitb, actually I was doing manually which means it is not perfect. However I could share my following steps like below:

  • Just pass through each header file and find the incomplete types.
  • Only forward declaration is enough and remove unnecessary header files.
  • If dependent header file is already included in the header file, remove it from implementation file.

This could be done by using IWYU as well btw.

@cngzhnp cngzhnp force-pushed the forward_declaration_for_sdk branch from 35d0ba3 to 53b5d01 Compare June 12, 2023 11:37
lalitb
lalitb reviewed Jun 13, 2023
View reviewed changes
examples/jaeger/main.cc Show resolved Hide resolved
examples/otlp/grpc_log_main.cc Show resolved Hide resolved
examples/otlp/grpc_log_main.cc Show resolved Hide resolved
@cngzhnp cngzhnp force-pushed the forward_declaration_for_sdk branch from 6079410 to bbe9ddf Compare June 14, 2023 10:36
@esigo esigo self-assigned this Jun 19, 2023
esigo
esigo approved these changes Jun 20, 2023
View reviewed changes
Copy link
Member

@esigo esigo left a comment

Choose a reason for hiding this comment

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

LGTM
thanks :)
nit: Jaeger exporter will be removed. no need to have changes there.

@esigo esigo added the ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) label Jun 20, 2023
lalitb
lalitb reviewed Jun 20, 2023
View reviewed changes
sdk/include/opentelemetry/sdk/logs/logger_provider.h
# include "opentelemetry/sdk/logs/processor.h"
# include "opentelemetry/nostd/string_view.h"
# include "opentelemetry/sdk/resource/resource.h"
# include "opentelemetry/version.h"
Copy link
Member

@lalitb lalitb Jun 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

nit - This file has reference to KeyValueIterable, - seems it is indirectly included. Not a blocker, we can fix this separately.

lalitb
lalitb reviewed Jun 20, 2023
View reviewed changes
sdk/include/opentelemetry/sdk/logs/multi_recordable.h
@@ -20,6 +19,8 @@ namespace sdk
{
namespace logs
{
class LogRecordProcessor;

Copy link
Member

Choose a reason for hiding this comment

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

This file has reference to opentelemetry::common::SystemTimestamp - seems to be indirectly included. Not a blocker, but can be fixed separately.

Copy link
Member

Choose a reason for hiding this comment

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

@lalitb

Good find.

I think we should automate this with include-what-you-use, because doing manual reviews checking all dependencies gets too tedious and error prone.

@marcalff marcalff changed the title Forward declaration is applied in SDK folder [SDK] Header files cleanup, use forward declarations Jun 20, 2023
@marcalff marcalff merged commit cfcda57 into open-telemetry:main Jun 20, 2023
@marcalff marcalff changed the title [SDK] Header files cleanup, use forward declarations [BUILD] SDK Header files cleanup, use forward declarations Jul 11, 2023
@chenrui333 chenrui333 mentioned this pull request Jul 14, 2023
Merged
@lalitb lalitb mentioned this pull request Jul 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcalff marcalff marcalff approved these changes

@esigo esigo esigo approved these changes

@lalitb lalitb lalitb approved these changes

Assignees

@esigo esigo

Labels
ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cngzhnp @lalitb @marcalff @esigo