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

fix: Use the correct opentracing plugin for Jaeger #2744

Merged
merged 2 commits into from
Jul 5, 2018
Merged

fix: Use the correct opentracing plugin for Jaeger #2744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
85d1742
fix: Use the correct opentracing plugin for Jaeger
mikebryant Jul 5, 2018
0e26cff
refactor: Remove unnecessary libthrift libraries, as Jaeger is static…
mikebryant Jul 5, 2018
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
2 changes: 1 addition & 1 deletion images/nginx/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# limitations under the License.

# 0.0.0 shouldn't clobber any released builds
TAG ?= 0.53
TAG ?= 0.54
REGISTRY ?= quay.io/kubernetes-ingress-controller
ARCH ?= $(shell go env GOARCH)
DOCKER ?= docker
Expand Down
19 changes: 11 additions & 8 deletions images/nginx/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,19 +283,22 @@ cmake -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTING=OFF ..
make
make install

# build zipkin lib
# build jaeger lib
cd "$BUILD_PATH/jaeger-client-cpp-$JAEGER_VERSION"
sed -i 's/-Werror//' CMakeLists.txt
mkdir .build
cd .build
cmake -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=1 -DBUILD_TESTING=OFF -DJAEGERTRACING_WITH_YAML_CPP=OFF -DJAEGERTRACING_BUILD_EXAMPLES=OFF ..
# Taken from https://github.com/jaegertracing/jaeger-client-cpp/blob/v0.4.1/scripts/build-plugin.sh
Copy link
Member

Choose a reason for hiding this comment

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

@rnburn is this ok? I was using the same steps in your repository https://github.com/opentracing-contrib/nginx-opentracing/blob/master/Dockerfile#L65

Copy link
Contributor

@rnburn rnburn Jul 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

That looks good.

Jaeger needs the JAEGERTRACING_WITH_YAML_CPP option enabled to be able to load a JSON configuration, which was probably what was causing trouble.

But BTW - since Jaeger links in everything statically, you should be able to remove the _3rdParty/Hunter/ directory when you're finished. It would make your image smaller.

Copy link
Member

Choose a reason for hiding this comment

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

@mikebryant can you implement what @rnburn mentioned?

Copy link
Member

Choose a reason for hiding this comment

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

@mikebryant last change and we are ready to merge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aledbf Isn't that already taken care of by the rm -rf "$BUILD_PATH" line later in the script, that cleans up all of the build stuff?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe most of it's taken care of, but you can delete this line

cp $HUNTER_INSTALL_DIR/lib/libthrift* /usr/local/lib

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see, sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

cat <<EOF > export.map
{
global:
OpenTracingMakeTracerFactory;
local: *;
};
EOF
cmake -DCMAKE_BUILD_TYPE=Release -DJAEGERTRACING_PLUGIN=ON -DBUILD_TESTING=OFF -DJAEGERTRACING_BUILD_EXAMPLES=OFF -DHUNTER_CONFIGURATION_TYPES=Release ..
make
make install

export HUNTER_INSTALL_DIR=$(cat _3rdParty/Hunter/install-root-dir)
echo "HUNTER_INSTALL_DIR: ${HUNTER_INSTALL_DIR}"
cp $HUNTER_INSTALL_DIR/lib/libthrift* /usr/local/lib
rm /usr/local/lib/libthrift*.a
mv libjaegertracing_plugin.so /usr/local/lib/libjaegertracing_plugin.so

# build zipkin lib
cd "$BUILD_PATH/zipkin-cpp-opentracing-$ZIPKIN_CPP_VERSION"
Expand Down
2 changes: 1 addition & 1 deletion internal/ingress/controller/template/template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -855,7 +855,7 @@ func buildOpentracing(input interface{}) string {
if cfg.ZipkinCollectorHost != "" {
buf.WriteString("opentracing_load_tracer /usr/local/lib/libzipkin_opentracing.so /etc/nginx/opentracing.json;")
} else if cfg.JaegerCollectorHost != "" {
buf.WriteString("opentracing_load_tracer /usr/local/lib/libjaegertracing.so /etc/nginx/opentracing.json;")
buf.WriteString("opentracing_load_tracer /usr/local/lib/libjaegertracing_plugin.so /etc/nginx/opentracing.json;")
}

buf.WriteString("\r\n")
Expand Down