-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
opentelemetry-cpp: add version 1.16.1, 1.17.0 #25902
opentelemetry-cpp: add version 1.16.1, 1.17.0 #25902
Conversation
@@ -195,7 +190,7 @@ def validate(self): | |||
|
|||
def build_requirements(self): | |||
if self.options.with_otlp_grpc or self.options.with_otlp_http: | |||
self.tool_requires("opentelemetry-proto/1.3.0") | |||
self.tool_requires("opentelemetry-proto/1.3.2") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
open-telemetry/opentelemetry-cpp#2991 changes in the documentation seem to imply that there's not need to map 1:1 unless a new version requires a minimum version, so after checking locally, old versions till compile with this, so I've simplified this @ArielGMachado, lmkwyt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without knowing much in depth about the opentelemetry-cpp
's internals, what I understand from documentation (https://github.com/open-telemetry/opentelemetry-cpp/blob/main/docs/maintaining-dependencies.md) is that opentelemetry-cpp
generates code from opentelemetry-proto
as part of the opentelemetry-cpp build. That are protobuf specs.
Each 3rd party version that uses in opentelemetry-cpp
is being indicated in the file third_party_release
of each release.
When upgrading opentelemetry-proto
to a newer release code can requires adjustment, so if a version requires code and a specific version of opentelemetry-proto
, it is indicated in the release.
According to the (Release info v1.16.0) , was upgraded to opentelemetry-proto/1.3.1
, and in the (Release info v1.17.0) was upgraded to opentelemetry-proto/1.3.2
.
I made the change in the recipe according to this principle to ensure the receipe compile each version with the exact dependency version they indicate in the release and not diverge in the Conan recipe from the official version of opentelemetry-cpp project.
I hope this helps you understand the reason for the change.
@@ -27,7 +27,6 @@ class OpenTelemetryCppConan(ConanFile): | |||
"with_stl": [True, False], | |||
"with_gsl": [True, False], | |||
"with_abseil": [True, False], | |||
"with_otlp": ["deprecated", True, False], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed so that the new versions don't have this from the get-go, and it's been deprecated long enough to warrant removal for old versions
Succesful compilation for the new options:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now looks good on my end, but will ask another team member to double check the proto
tool_require
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checked both new versions, and many things changed.
- Good job by updating cpp_info with requirements related to OTLP_FILE
- It would be nice a way to disable test for OTLP_FILE: open-telemetry/opentelemetry-cpp@v1.14.2...v1.16.1#diff-d2b47df314846874caa57ba4f0aa5aefbe04aa1de090b8f4c53267c12476eddbR489
- Version 1.17.0 add new component: opentelemetry_exporter_in_memory_metric - open-telemetry/opentelemetry-cpp@v1.16.1...v1.17.0#diff-d1a44fdeb6f1751db9732c7a726d8c0bbcd27be11f56ad3c3c4648491b9d877dR19. So far, this recipe only covers exporter_in_memory, not memory_metric
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Co-authored-by: Abril Rincón Blanco <rubenrb@jfrog.com> Co-authored-by: Abril Rincón Blanco <git@rinconblanco.es>
Co-authored-by: Abril Rincón Blanco <rubenrb@jfrog.com> Co-authored-by: Abril Rincón Blanco <git@rinconblanco.es>
Co-authored-by: Abril Rincón Blanco <rubenrb@jfrog.com> Co-authored-by: Abril Rincón Blanco <git@rinconblanco.es>
Summary
Changes to recipe: opentelemetry-cpp/*
Notice: this PR requires PR #25901 be merged first because dependency of opentelemetry-proto versions
Motivation
There are breaking changes in 1.17.0
There are several bugfixes in 1.16.1
Details
open-telemetry/opentelemetry-cpp@v1.16.1...v1.17.0
open-telemetry/opentelemetry-cpp@v1.14.2...v1.16.1