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

WIP: Comsume proto files from git submodule and build them in a separate module #504

Merged
merged 9 commits into from
Aug 22, 2019

Conversation

pavolloffay
Copy link
Member

@pavolloffay pavolloffay commented Aug 20, 2019

Resolves #232
Replaces #483
Depends on open-telemetry/opentelemetry-proto#26, after the merge I will change the GIT URL to point to the upstream.

WIP: because the git submodule is based on my fork of proto repository https://github.com/pavolloffay/opentelemetry-proto

Signed-off-by: Pavol Loffay ploffay@redhat.com

@pavolloffay
Copy link
Member Author

@bogdandrutu @songy23 could you please have a look?

@songy23
Copy link
Member

songy23 commented Aug 20, 2019

Configuration errors: 1 error occurred:

* In step 3 definition: Invalid step structure (expected string or map, got config.StepDescription)

Please fix.

.circleci/config.yml Outdated Show resolved Hide resolved
dependencies {
api project(':opentelemetry-api')

implementation "com.google.protobuf:protobuf-java:${protobufVersion}",
Copy link
Member

Choose a reason for hiding this comment

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

These 2 can be defined in the libraries to be consistent with other dependencies.

proto/build.gradle Outdated Show resolved Hide resolved
exporters/jaeger/build.gradle Outdated Show resolved Hide resolved
@bogdandrutu
Copy link
Member

If you rebase you will fix the android errors.

@@ -108,6 +108,8 @@ subprojects {
grpcVersion = '1.20.0'
autoValueVersion = '1.6.2'
opentracingVersion = '0.33.0'
protobufVersion = '3.7.1'
Copy link
Contributor

Choose a reason for hiding this comment

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

+1. Not all the artifacts will use protobufVersion (i.e. the API itself), but this way we hopefully don't have to re-declare this version again and again ;)

…odule

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@codecov-io
Copy link

codecov-io commented Aug 21, 2019

Codecov Report

Merging #504 into master will decrease coverage by 0.2%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #504      +/-   ##
============================================
- Coverage     78.55%   78.34%   -0.21%     
+ Complexity      561      560       -1     
============================================
  Files            65       65              
  Lines          1926     1926              
  Branches        188      188              
============================================
- Hits           1513     1509       -4     
- Misses          355      357       +2     
- Partials         58       60       +2
Impacted Files Coverage Δ Complexity Δ
...y/sdk/trace/export/BatchSampledSpansProcessor.java 89.77% <0%> (-2.28%) 6% <0%> (ø)
...ntelemetry/sdk/trace/RecordEventsReadableSpan.java 98.79% <0%> (-1.21%) 55% <0%> (-1%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66a15d3...e1bd471. Read the comment docs.

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@pavolloffay
Copy link
Member Author

I have submitted PR to proto repository to remove the gradle config and move files to the root directory open-telemetry/opentelemetry-proto#26

.circleci/config.yml Show resolved Hide resolved
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@bogdandrutu bogdandrutu merged commit b13457b into open-telemetry:master Aug 22, 2019
@@ -0,0 +1,3 @@
[submodule "proto/src/main/proto"]
path = proto/src/main/proto
url = https://github.com/pavolloffay/opentelemetry-proto.git
Copy link
Member Author

Choose a reason for hiding this comment

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

@bogdandrutu I have marked this PR as WIP with depends on open-telemetry/opentelemetry-proto#26.

Since this is merged I will create an issue to change to upsream repo once it's ready

@songy23
Copy link
Member

songy23 commented Aug 22, 2019

Can someone advice on how to do Gradle build locally after this change? I tried ./gradlew build on my workstation and Gradle complained about cannot find protos:

error: package io.opentelemetry.proto.trace.v1 does not exist
import io.opentelemetry.proto.trace.v1.Span;
...
30 errors
FAILURE: Build failed with an exception.

@bogdandrutu
Copy link
Member

Use make

@songy23
Copy link
Member

songy23 commented Aug 22, 2019

Cool that worked. Created #507 to reflect this change in CONTRIBUTING.md.

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.

None yet

5 participants