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] OTLP HTTP Exporter has build warnings in maintainer mode #1943

Merged
merged 9 commits into from
Feb 3, 2023
Merged

[BUILD] OTLP HTTP Exporter has build warnings in maintainer mode #1943

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f2f4297
Fix #1942
marcalff Jan 30, 2023
7503f40
Added CHANGELOG, fixed CI
marcalff Jan 30, 2023
c4ddff1
Fixed typo
marcalff Jan 30, 2023
9f33e0c
Used curly braces {} for environment variables, instead of ()
marcalff Jan 30, 2023
571ef39
Merge branch 'main' into fix_warnings_otlp_1942
marcalff Jan 30, 2023
053a0aa
Abandon make -j,
marcalff Jan 31, 2023
63a53e1
Restored make -j, with the proper syntax : $(nproc)
marcalff Jan 31, 2023
feda056
Merge branch 'open-telemetry:main' into fix_warnings_otlp_1942
marcalff Jan 31, 2023
43b1124
Merge branch 'open-telemetry:main' into fix_warnings_otlp_1942
marcalff Jan 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,11 @@ jobs:
CC: /usr/bin/gcc-12
CXX: /usr/bin/g++-12
GOOGLETEST_VERSION: 1.12.1
PROTOBUF_VERSION: 21.12
run: |
sudo -E ./ci/setup_cmake.sh
sudo -E ./ci/setup_ci_environment.sh
sudo -E ./ci/install_protobuf.sh
- name: run cmake gcc (maintainer mode)
env:
CC: /usr/bin/gcc-12
Expand All @@ -64,9 +66,11 @@ jobs:
CC: /usr/bin/clang-14
CXX: /usr/bin/clang++-14
GOOGLETEST_VERSION: 1.12.1
PROTOBUF_VERSION: 21.12
run: |
sudo -E ./ci/setup_cmake.sh
sudo -E ./ci/setup_ci_environment.sh
sudo -E ./ci/install_protobuf.sh
- name: run cmake clang (maintainer mode)
env:
CC: /usr/bin/clang-14
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ Increment the:
* [MAINTAINER DOC] Define and document a deprecation process,
[DEPRECATION] Deprecate the Jaeger exporter,
implemented by [#1923](https://github.com/open-telemetry/opentelemetry-cpp/pull/1923)
* [BUILD] OTLP HTTP Exporter has build warnings in maintainer mode
[#1943](https://github.com/open-telemetry/opentelemetry-cpp/pull/1943)

Deprecations:

Expand Down
2 changes: 2 additions & 0 deletions ci/do_ci.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ elif [[ "$1" == "cmake.maintainer.test" ]]; then
cd "${BUILD_DIR}"
rm -rf *
cmake -DCMAKE_BUILD_TYPE=Debug \
-DWITH_OTLP=ON \
-DWITH_OTLP_HTTP=ON \
-DWITH_PROMETHEUS=ON \
-DWITH_EXAMPLES=ON \
-DWITH_EXAMPLES_HTTP=ON \
Expand Down
34 changes: 30 additions & 4 deletions ci/install_protobuf.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,38 @@

set -e

[ -z "${PROTOBUF_VERSION}" ] && export PROTOBUF_VERSION="3.11.4"
[ -z "${PROTOBUF_VERSION}" ] && export PROTOBUF_VERSION="21.12"

#
# Note
#
# protobuf uses two release number schemes,
# for example 3.21.12 and 21.12,
# and both tags corresponds to the same commit:
#
# commit f0dc78d7e6e331b8c6bb2d5283e06aa26883ca7c (HEAD -> release-3.21.12, tag: v3.21.12, tag: v21.12)
# Author: Protobuf Team Bot <protobuf-team-bot@google.com>
# Date: Mon Dec 12 16:03:12 2022 -0800
#
# Updating version.json and repo version numbers to: 21.12
#
# tag v21.12 corresponds to the 'protoc version', or repo version
# tag v3.21.12 corresponds to the 'cpp version'
#
# protobuf-cpp-3.21.12.tar.gz:
# - is provided under releases/download/v21.12
# - is no longer provided under releases/download/v3.21.12,
#
# Use the "repo version number" (PROTOBUF_VERSION=21.12)
# when calling this script
#

export CPP_PROTOBUF_VERSION="3.${PROTOBUF_VERSION}"

cd /
wget https://github.com/google/protobuf/releases/download/v${PROTOBUF_VERSION}/protobuf-cpp-${PROTOBUF_VERSION}.tar.gz
tar zxf protobuf-cpp-${PROTOBUF_VERSION}.tar.gz --no-same-owner
cd protobuf-${PROTOBUF_VERSION}
wget https://github.com/google/protobuf/releases/download/v${PROTOBUF_VERSION}/protobuf-cpp-${CPP_PROTOBUF_VERSION}.tar.gz
tar zxf protobuf-cpp-${CPP_PROTOBUF_VERSION}.tar.gz --no-same-owner
cd protobuf-${CPP_PROTOBUF_VERSION}
./configure
make && make install
Copy link
Member

Choose a reason for hiding this comment

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

We may use make -j || make && make install to reduce the compiling time.

Copy link
Member Author

@marcalff marcalff Feb 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @owent

This part changed a bit back and forth, not sure which commit you looked at.

Currently the code is make -j $(nproc) && make install

When using make -j ${nproc}, which was wrong because there is no nproc environment variable, make -j was executed alone ... this killed the github CI.

make -j $(nproc), which is used in different places already, correctly invokes the nproc utility to return the number of cores. This works fine in CI now.

So, parallel make is used already, precisely to reduce the compiling time.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation.LGTM now.

ldconfig
2 changes: 1 addition & 1 deletion exporters/otlp/src/otlp_metric_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ sdk::metrics::AggregationTemporality OtlpMetricUtils::DeltaTemporalitySelector(
}

sdk::metrics::AggregationTemporality OtlpMetricUtils::CumulativeTemporalitySelector(
sdk::metrics::InstrumentType instrument_type) noexcept
sdk::metrics::InstrumentType /* instrument_type */) noexcept
{
return sdk::metrics::AggregationTemporality::kCumulative;
}
Expand Down
17 changes: 12 additions & 5 deletions sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage
SyncMetricStorage(InstrumentDescriptor instrument_descriptor,
const AggregationType aggregation_type,
const AttributesProcessor *attributes_processor,
nostd::shared_ptr<ExemplarReservoir> &&exemplar_reservoir,
nostd::shared_ptr<ExemplarReservoir> &&exemplar_reservoir
OPENTELEMETRY_MAYBE_UNUSED,
const AggregationConfig *aggregation_config)
: instrument_descriptor_(instrument_descriptor),
attributes_hashmap_(new AttributesHashMap()),
Expand All @@ -48,7 +49,9 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage
};
}

void RecordLong(int64_t value, const opentelemetry::context::Context &context) noexcept override
void RecordLong(int64_t value,
const opentelemetry::context::Context &context
OPENTELEMETRY_MAYBE_UNUSED) noexcept override
{
if (instrument_descriptor_.value_type_ != InstrumentValueType::kLong)
{
Expand All @@ -63,7 +66,8 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage

void RecordLong(int64_t value,
const opentelemetry::common::KeyValueIterable &attributes,
const opentelemetry::context::Context &context) noexcept override
const opentelemetry::context::Context &context
OPENTELEMETRY_MAYBE_UNUSED) noexcept override
{
if (instrument_descriptor_.value_type_ != InstrumentValueType::kLong)
{
Expand All @@ -78,7 +82,9 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage
attributes_hashmap_->GetOrSetDefault(attr, create_default_aggregation_)->Aggregate(value);
}

void RecordDouble(double value, const opentelemetry::context::Context &context) noexcept override
void RecordDouble(double value,
const opentelemetry::context::Context &context
OPENTELEMETRY_MAYBE_UNUSED) noexcept override
{
if (instrument_descriptor_.value_type_ != InstrumentValueType::kDouble)
{
Expand All @@ -93,7 +99,8 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage

void RecordDouble(double value,
const opentelemetry::common::KeyValueIterable &attributes,
const opentelemetry::context::Context &context) noexcept override
const opentelemetry::context::Context &context
OPENTELEMETRY_MAYBE_UNUSED) noexcept override
{
#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW
exemplar_reservoir_->OfferMeasurement(value, attributes, context,
Expand Down