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

Moved otlp_grpc_utils.cc to opentelemetry_exporter_otlp_grpc_client. #1829

Merged
merged 4 commits into from
Dec 1, 2022

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Dec 1, 2022

Fixes #1826

Changes

Moved otlp_grpc_utils.cc to opentelemetry_exporter_otlp_grpc_client.

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

@lalitb lalitb requested a review from a team December 1, 2022 20:10
@esigo
Copy link
Member

esigo commented Dec 1, 2022

Shall we make this consistent with our bazel?

name = "otlp_grpc_utils",
srcs = [
"src/otlp_grpc_utils.cc",
],
hdrs = [
"include/opentelemetry/exporters/otlp/otlp_grpc_utils.h",
],
strip_include_prefix = "include",
tags = ["otlp"],
deps = [
"//api",
"//sdk:headers",
"@com_github_grpc_grpc//:grpc++",
],
)

@lalitb
Copy link
Member Author

lalitb commented Dec 1, 2022

Yes, good point to be consistent with bazel. Will do so

@codecov
Copy link

codecov bot commented Dec 1, 2022

Codecov Report

Merging #1829 (5316eb1) into main (82d8cb4) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1829   +/-   ##
=======================================
  Coverage   85.73%   85.73%           
=======================================
  Files         171      171           
  Lines        5240     5240           
=======================================
  Hits         4492     4492           
  Misses        748      748           

@esigo esigo added area:exporter:otlp OpenTelemetry Protocol (OTLP) Exporter area:build OpenTelemetry build labels Dec 1, 2022
@lalitb
Copy link
Member Author

lalitb commented Dec 1, 2022

Or should we have single library otlp_grpc_client for both bazel and cmake, as in the latest changes in this PR - I don't see reason to have two different libraries for gRPC stuff.

@esigo
Copy link
Member

esigo commented Dec 1, 2022

Or should we have single library otlp_grpc_client for both bazel and cmake, as in the latest changes in this PR - I don't see reason to have two different libraries for gRPC stuff.

Yeah agree. doesn't make sense to have two libs.

@marcalff
Copy link
Member

marcalff commented Dec 1, 2022

Nit - In the PR description and title:

Added otlp_grpc_utils.cc in the opentelemetry_exporter_otlp_grpc.

->

Moved otlp_grpc_utils.cc to opentelemetry_exporter_otlp_grpc_client.

@ThomsonTan
Copy link
Contributor

Or should we have single library otlp_grpc_client for both bazel and cmake, as in the latest changes in this PR - I don't see reason to have two different libraries for gRPC stuff.

Yeah agree. doesn't make sense to have two libs.

+1. Put all grpc specific code in otlp_grpc_client makes sense for both bazel and cmake.

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.

Thanks for the fix

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 for the fix :)

@lalitb lalitb changed the title Add otlp_grpc_utils.cc in the opentelemetry_exporter_otlp_grpc Moved otlp_grpc_utils.cc to opentelemetry_exporter_otlp_grpc_client. Dec 1, 2022
@lalitb lalitb merged commit a24488f into open-telemetry:main Dec 1, 2022
yxue pushed a commit to yxue/opentelemetry-cpp that referenced this pull request Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:build OpenTelemetry build area:exporter:otlp OpenTelemetry Protocol (OTLP) Exporter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Build] Build break when using WITH_OTLP_HTTP=ON, WITH_OTLP_GRPC=OFF
4 participants